netdev - Re: [PATCH 0/2] [BUG FIXES - 3.10.27] sit: More backports

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, 30 Jan 2014 10:28:43 +0100
From:	Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	stable <stable@...r.kernel.org>,
	Clark Williams <williams@...hat.com>,
	"Luis Claudio R. Goncalves" <lclaudio@...g.org>,
	John Kacur <jkacur@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH 0/2] [BUG FIXES - 3.10.27] sit: More backports

Le 29/01/2014 21:48, Steven Rostedt a écrit :
> On Wed, 29 Jan 2014 17:04:12 +0100
> Nicolas Dichtel <nicolas.dichtel@...nd.com> wrote:
>
>> Your patch serie seems to be the good way to go (note that patch 1/2 does not
>> compile) but I think the fix is smaller because we don't have x-netns.
>>
>> Here is my proposal, if you agree, I will send the same patch for ip6_tunnnel,
>> which have the netns leak.
>>
>
> Hold on. Seems that the kernels that were being tested in QA had more
> code than what I was testing. Clark had backported "sit: fix use after
> free of fb_tunnel_dev" and that was what was causing the
> unlist_netdevice() to be missed.
>
> When I started working on vanilla 3.10.27 as well, I first did my
> original patch (which just removes the call to
> unregister_netdevice_queue() from sit_exit_net()). I asked to have that
> added to our kernel for testing, and they told me it was already there
> via Clark's backport. Then I did the full backport as well and looked
> for the leak. I'm now thinking that the full backport is not needed as
> that was what caused the leak.
>
> According to commit 9434266f2c645d4fcf62a03a8e36ad8075e37943 "sit: fix
> use after free of fb_tunnel_dev", it states:
>
>      Bug: The fallback device is created in sit_init_net and assumed to
>      be freed in sit_exit_net. First, it is dereferenced in that
>      function, in sit_destroy_tunnels:
>
>              struct net *net = dev_net(sitn->fb_tunnel_dev);
>
>      Prior to this, rtnl_unlink_register has removed all devices that
>      match rtnl_link_ops == sit_link_ops.
>
>      Commit 205983c43700 added the line
>
>      +       sitn->fb_tunnel_dev->rtnl_link_ops = &sit_link_ops;
>
>      which cases the fallback device to match here and be freed before it
>      is last dereferenced.
>
> Commit 205983c43700 was backported to 3.10, but without commit
> 5e6700b3bf98 "sit: add support of x-netns" which was what added the
>
>    net = dev_net(sitn->fb_tunnel_dev);
>
> Which looks to me that the only reason I need to port back commit
> 5e6700b3bf98 is if I add the full backport of 9434266f2c645d4f.
>
> Seems to me that my original patch may be good enough. The one that I
> said this series obsoletes.
>
> Note, I've talked with the people that are doing the testing, and I'm
> having them revert all changes except for that one fix and rerun the
> tests again. I should know the results by tomorrow.
>
> Let me know if "sit: fix use after free of fb_tunnel_dev" still needs
> to be backported due to some other way that the fallback device can be
> dereferenced after being freed.

Steve, I think the patch I sent yesterday is the good fix. At the end, it's
a backport of Willem's patch. Note that he also ack that patch.
The first version you sent (which removes
unregister_netdevice_queue(sitn->fb_tunnel_dev, &list)) will introduce a
memory leak when the user destroy a netns.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html