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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 3 Dec 2020 19:01:56 +0100 From: Guillaume Nault <gnault@...hat.com> To: Tom Parkin <tparkin@...alix.com> Cc: netdev@...r.kernel.org, jchapman@...alix.com Subject: Re: [PATCH v2 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls On Thu, Dec 03, 2020 at 11:57:18AM +0000, Tom Parkin wrote: > On Thu, Dec 03, 2020 at 01:23:18 +0100, Guillaume Nault wrote: > > On Tue, Dec 01, 2020 at 11:52:49AM +0000, Tom Parkin wrote: > > > + if (!pchb) { > > > + write_unlock_bh(&pch->upl); > > > + return -EINVAL; > > > > I'm not sure I'd consider this case as an error. > > To be honest I'd probably tend agree with you, but I was seeking to > maintain consistency with how PPPIOCCONNECT/PPPIOCDISCONN behave. The > latter returns EINVAL if the channel isn't connected to an interface. Indeed, that makes sense. I didn't think about that. I was mostly thinking about the case where ->bridge was concurently reset by another ppp_unbridge_channels(), which doesn't look like an error to me. But we can let userspace responsible for properly using the API (or ignoring EINVAL when they don't). > If you feel strongly I'm happy to change it but IMO it's better to be > consistent with existing ioctl calls. I don't feel strongly about it :). > > If there's no bridged channel, there's just nothing to do. > > Furthermore, there might be situations where this is not really an > > error (see the possible race below). > > > > > + } > > > + RCU_INIT_POINTER(pch->bridge, NULL); > > > + write_unlock_bh(&pch->upl); > > > + > > > + write_lock_bh(&pchb->upl); > > > + RCU_INIT_POINTER(pchb->bridge, NULL); > > > + write_unlock_bh(&pchb->upl); > > > + > > > + synchronize_rcu(); > > > + > > > + if (refcount_dec_and_test(&pch->file.refcnt)) > > > + ppp_destroy_channel(pch); > > > > I think that we could have a situation where pchb->bridge could be > > different from pch. > > If ppp_unbridge_channels() was also called concurrently on pchb, then > > pchb->bridge might have been already reset. And it might have dropped > > the reference it had on pch. In this case, we'd erroneously decrement > > the refcnt again. > > > > In theory, pchb->bridge might even have been reassigned to a different > > channel. So we'd reset pchb->bridge, but without decrementing the > > refcnt of the channel it pointed to (and again, we'd erroneously > > decrement pch's refcount instead). > > > > So I think we need to save pchb->bridge to a local variable while we > > hold pchb->upl, and then drop the refcount of that channel, instead of > > assuming that it's equal to pch. > > Ack, yes. > > The v1 series protected against this, although by nesting locks :-| Well, I think the v1 could deadlock in this situation. The RFC was immune to this problem, as it didn't modify ->bridge on pchb. > I think in the case that pchb->bridge != pch, we probably want to > leave pchb alone, so: > > 1. Don't unset the pchb->bridge pointer > 2. Don't drop the pch reference (pchb doesn't hold a reference on pch > because pchb->bridge != pch) > > This is on the assumption that pchb has been reassigned -- in that > scenario we don't want to mess with pchb at all since it'll break the > other bridge instance. Yes you're right. Thanks!
Powered by blists - more mailing lists