diff mbox

Possible double-free in the usbnet driver

Message ID CA+fCnZfS51F7WZEM1YTSPDMWSzvBTJWGf5cRWv5LrNCSOf_-qA@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Andrey Konovalov March 4, 2016, 10:26 p.m. UTC 
On Sat, Mar 5, 2016 at 12:26 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> [ Moving this to proper lists ]
>
> On Thu, Mar 3, 2016 at 4:19 PM, Andrey Konovalov <andreyknvl@gmail.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:


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

Comments

Oliver Neukum March 4, 2016, 10:42 p.m. UTC | #1 
On Sat, 2016-03-05 at 01:26 +0300, Andrey Konovalov wrote:
> 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.

Do you have lsusb?

	Regards
		Oliver
Andrey Konovalov March 4, 2016, 11 p.m. UTC | #2 
On Sat, Mar 5, 2016 at 1:42 AM, Oliver Neukum <oneukum@suse.de> wrote:
> On Sat, 2016-03-05 at 01:26 +0300, Andrey Konovalov wrote:
>> 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.
>
> Do you have lsusb?

You mean inside the vm?
I do.

>
>         Regards
>                 Oliver
>
>
Andrey Konovalov March 4, 2016, 11:22 p.m. UTC | #3 
On Sat, Mar 5, 2016 at 2:00 AM, Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Sat, Mar 5, 2016 at 1:42 AM, Oliver Neukum <oneukum@suse.de> wrote:
>> On Sat, 2016-03-05 at 01:26 +0300, Andrey Konovalov wrote:
>>> 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.
>>
>> Do you have lsusb?
>
> You mean inside the vm?
> I do.

Or did you want the faulty device descriptor itself?

I used this:

Speed High
Bus 004 Device 003: ID 0bdb:1911
Device Descriptor:
  bLength                 18
  bDescriptorType          1
  bcdUSB                2.00
  bDeviceClass             2 Communications
  bDeviceSubClass          0
  bDeviceProtocol          0
  bMaxPacketSize0         64
  idVendor            0x0000
  idProduct           0x0000
  bcdDevice             0.00
  iManufacturer            1
  iProduct                 2
  iSerial                  3
  bNumConfigurations       1
Configuration Descriptor:
  bLength                  9
  bDescriptorType          2
  wTotalLength           371
  bNumInterfaces          11
  bConfigurationValue      1
  iConfiguration           4
  bmAttributes          0xe0
    Self Powered
    Remote Wakeup
  bMaxPower                0mA
Interface Descriptor:
  bLength                  9
  bDescriptorType          4
  bInterfaceNumber         6
  bAlternateSetting        0
  bNumEndpoints            1
  bInterfaceClass          2 Communications
  bInterfaceSubClass      13
  bInterfaceProtocol       0
  iInterface              11

>
>>
>>         Regards
>>                 Oliver
>>
>>
diff mbox

Patch

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;