netdev - Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160225191655.GA1462@alphalink.fr>
Date:	Thu, 25 Feb 2016 20:16:55 +0100
From:	Guillaume Nault <g.nault@...halink.fr>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, paulus@...ba.org
Subject: Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in
 ppp_ioctl()

On Wed, Feb 24, 2016 at 03:32:02PM -0500, David Miller wrote:
> From: Guillaume Nault <g.nault@...halink.fr>
> 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 <g.nault@...halink.fr>
>  ...
> > 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?