netdev - Re: [Patch net] ppp: defer netns reference release for ppp channel

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]

Date:	Thu, 28 Jul 2016 12:33:58 +0200
From:	Guillaume Nault <g.nault@...halink.fr>
To:	Cyrill Gorcunov <gorcunov@...il.com>
Cc:	Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
	Matt.Bennett@...iedtelesis.co.nz,
	Paul Mackerras <paulus@...ba.org>, linux-ppp@...r.kernel.org
Subject: Re: [Patch net] ppp: defer netns reference release for ppp channel

On Wed, Jul 06, 2016 at 03:25:15PM +0300, Cyrill Gorcunov wrote:
> On Wed, Jul 06, 2016 at 11:26:02AM +0300, Cyrill Gorcunov wrote:
> > On Tue, Jul 05, 2016 at 10:12:36PM -0700, Cong Wang wrote:
> > > Matt reported that we have a NULL pointer dereference
> > > in ppp_pernet() from ppp_connect_channel(),
> > > i.e. pch->chan_net is NULL.
> > > 
> > > This is due to that a parallel ppp_unregister_channel()
> > > could happen while we are in ppp_connect_channel(), during
> > > which pch->chan_net set to NULL. Since we need a reference
> > > to net per channel, it makes sense to sync the refcnt
> > > with the life time of the channel, therefore we should
> > > release this reference when we destroy it.
> > > 
> > > Fixes: 1f461dcdd296 ("ppp: take reference on channels netns")
> > > Reported-by: Matt Bennett <Matt.Bennett@...iedtelesis.co.nz>
> > > Cc: Paul Mackerras <paulus@...ba.org>
> > > Cc: linux-ppp@...r.kernel.org
> > > Cc: Guillaume Nault <g.nault@...halink.fr>
> > > Cc: Cyrill Gorcunov <gorcunov@...nvz.org>
> > > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> > > ---
> > 
> > Hi Cong! I may be wrong, but this doesn't look right in general.
> > We take the net in ppp_register_channel->ppp_register_net_channel
> > and (name) context implies that ppp_unregister_channel does
> > the reverse. Maybe there some sync point missed? I'll review
> > in detail a bit later.
> 
> After staring more I think the patch should be fine as a fix
> since implementing sync with ppp_[re|un]register_channel and
> ppp_ioctl might need a way more work.
> 

[Sorry for arriving so late in the game, I was offline the last 3 weeks]

I agree having some symmetry between the creation and deletion
processes would be nice and would make the code easier to reason about.
Actually, I released the channel netns in ppp_unregister_channel() for
exactly this reason (and failed to spot this race).

But the code is already quite asymmetric and it's certainly too late to
move away from this scheme now. So releasing the channel netns in
ppp_destroy_channel() is in line with ppp_generic's architecture. Other
data are handled this way: e.g. channel_count is incremented in
ppp_register_net_channel() and decremented in ppp_destroy_channel()).

Thank you all for testing and fixing this issue!

Guillaume