Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[David Miller]

From: Guillaume Nault 
Date: Thu, 25 Feb 2016 20:16:55 +0100

> Understood. Just to be sure, does patch #2 falls under lack of
> demonstration? Or should I repost it separately?

Please repost it if you feel this way, with the race and corruption
possibility explained in detail in the commit log message.

Thanks.

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

On Wed, Feb 24, 2016 at 03:32:02PM -0500, David Miller wrote:
> From: Guillaume Nault 
> Date: Mon, 22 Feb 2016 20:47:13 +0100
> 
> > PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> > and ppp_recv_lock() respectively.
> > Therefore ppp_ioctl() must hold the xmit and recv locks before
> > concurrently updating ppp->mru.
> > 
> > Signed-off-by: Guillaume Nault 
>  ...
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index fc8ad00..4d342ae 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int 
> > cmd, unsigned long arg)
> > case PPPIOCSMRU:
> > if (get_user(val, p))
> > break;
> > +   ppp_lock(ppp);
> > ppp->mru = val;
> > +   ppp_unlock(ppp);
> > +
> 
> I see no bug here at all.
> 
> The store here is atomic, and all of those mentioned code paths only
> read the MRU once and then use that value for the duration of the
> rest of the processing of that PPP frame.
> 
Ok, I didn't think we could assume atomic stores for int on all arch.

> No possible corruptions or misbehavior can occur and I therefore think
> the lack of locking here is completely legitimate.
> 
Then this is also legitimate for most of the other fields considered in
this series. I'll drop the patches.

One exception is the n_channels and flags fields (patch #2). The update
side is done with read-modify-write instructions ('ppp->flags &= ~XXX'
in ppp_ccp_closed(), '++ppp->n_channels' in ppp_connect_channel()). So
locking should be required. I haven't succeeded in triggering any
misbehaviour from userspace though.

> You absolutely must demonstrate a case of corruption or misbehavior
> when you want to add supposedly "missing locking".  Otherwise I'll have
> a hard time accepting your changes.  This is especially for a subsystem
> that as been around as long as PPP.
Understood. Just to be sure, does patch #2 falls under lack of
demonstration? Or should I repost it separately?

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[David Miller]

From: Guillaume Nault 
Date: Mon, 22 Feb 2016 20:47:13 +0100

> PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> and ppp_recv_lock() respectively.
> Therefore ppp_ioctl() must hold the xmit and recv locks before
> concurrently updating ppp->mru.
> 
> Signed-off-by: Guillaume Nault 
 ...
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index fc8ad00..4d342ae 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int 
> cmd, unsigned long arg)
>   case PPPIOCSMRU:
>   if (get_user(val, p))
>   break;
> + ppp_lock(ppp);
>   ppp->mru = val;
> + ppp_unlock(ppp);
> +

I see no bug here at all.

The store here is atomic, and all of those mentioned code paths only
read the MRU once and then use that value for the duration of the
rest of the processing of that PPP frame.

No possible corruptions or misbehavior can occur and I therefore think
the lack of locking here is completely legitimate.

You absolutely must demonstrate a case of corruption or misbehavior
when you want to add supposedly "missing locking".  Otherwise I'll have
a hard time accepting your changes.  This is especially for a subsystem
that as been around as long as PPP.

[PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
and ppp_recv_lock() respectively.
Therefore ppp_ioctl() must hold the xmit and recv locks before
concurrently updating ppp->mru.

Signed-off-by: Guillaume Nault 
---
 drivers/net/ppp/ppp_generic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..4d342ae 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, 
unsigned long arg)
case PPPIOCSMRU:
if (get_user(val, p))
break;
+   ppp_lock(ppp);
ppp->mru = val;
+   ppp_unlock(ppp);
+
err = 0;
break;
 
-- 
2.7.0