netdev - Re: Possible double-free in the usbnet driver

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:	Sat, 5 Mar 2016 01:26:37 +0300
From:	Andrey Konovalov <andreyknvl@...il.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Oliver Neukum <oneukum@...e.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kostya Serebryany <kcc@...gle.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	USB list <linux-usb@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: Possible double-free in the usbnet driver

On Sat, Mar 5, 2016 at 12:26 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> [ Moving this to proper lists ]
>
> On Thu, Mar 3, 2016 at 4:19 PM, Andrey Konovalov <andreyknvl@...il.com> wrote:
>>
>> I found another double-free, this time in the usbnet driver.
>
> Hmm. It doesn't look like a double free to me, at least from the logs
> you attached.
>
>> Whenever the `bind()` function fails (drivers/net/usb/usbnet.c:1676) when
>> called from `usbnet_probe()` (and it can fail due to a invalid usb descriptor),
>> `free_netdev()` is called for the `net` device (drivers/net/usb/usbnet.c:1772).
>> Then, `free_netdev(net)` is called again in `usbnet_disconnect()`
>> (drivers/net/usb/usbnet.c:1570) causing a double-free.
>
> The KASAN report says that it's a use-after-free in the kworker
> thread: the net device got free'd at the end of usbnet_probe(), but
> some work-struct was apparently active at the time.
>
> There might be a double free later that isn't in your report, though.
> Do you have the data for that?

I uploaded full kernel log here:
https://gist.github.com/xairy/6a244871959187209fdb

My initial idea was that an object is freed by free_netdev(), then
allocated somewhere during execution of kworker-related code and then
this object gets freed by free_netdev() again and that's why we have
such use-after-free reports. That didn't explain the deallocation
stack trace in the report, but I thought this was due to a
racy-use-after-free.

I just added the following debug output:

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 0b0ba7e..f7e1415 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1567,6 +1567,7 @@ void usbnet_disconnect (struct usb_interface *intf)
        usb_free_urb(dev->interrupt);
        kfree(dev->padding_pkt);

+       pr_err("usbnet_disconnect(): freeing netdev: %p\n", net);
        free_netdev(net);
 }
 EXPORT_SYMBOL_GPL(usbnet_disconnect);
@@ -1769,6 +1770,7 @@ out3:
        if (info->unbind)
                info->unbind (dev, udev);
 out1:
+       pr_err("usbnet_probe(): freeing netdev: %p\n", net);
        free_netdev(net);
 out:
        return status;

and when I run the vm and connect the device I get:

[   23.672662] cdc_ncm 1-1:1.6: bind() failure
[   23.673447] usbnet_probe(): freeing netdev: ffff88006ab48000
[   23.675822] usbnet_probe(): freeing netdev: ffff88006ab48000

So this seems to be a double-free (or at least a double free_netdev()
call), but the object gets freed twice from usbnet_probe() and not
from usbnet_disconnect(), so you're right that the latter doesn't get
called. I'm not sure how usbnet_probe() ends up being called twice.

>
> But I didn't think we even called the disconnect() function if the
> "bind()" failed, so I don't think that one should free it. Greg?
>
> So it *sounds* to me like the usbnet "bind()" routine ended up
> returning an error, but doing so *after* it had already activated the
> structure somehow.
>
> Which particular usbnet bind routine is this? There are multiple
> sub-drivers for usbnet that all do different things.

cdc_ncm_bind()

>
> For example, it *looks* like the cdc_ncm_bind() will have done a
> usbnet_link_change() even if the bind fails. So now we've done a
> usbnet_defer_kevent() even though we're failing, and then that sets
> the ball rolling to later touch the netdev that we're freeing due to
> the failure.
>
> But I may be *entirely* misreading this thing.
>
> Anyway, I'm cc'ing the usbnet people who actually know the code (and netdev).
>
> The proper fix may be to just cancel any work that might have been set
> up before freeing. Or maybe that netdev *does* get free'd later some
> other way properly. Let's see what the experts on the usbnet driver
> say.
>
>                   Linus