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
| ||
|
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?
Powered by blists - more mailing lists