Re: Possible double-free in the usbnet driver

Re: Possible double-free in the usbnet driver

[Linus Torvalds]

[ Moving this to proper lists ]

On Thu, Mar 3, 2016 at 4:19 PM, Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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?

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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

On Sat, Mar 5, 2016 at 12:26 AM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> [ Moving this to proper lists ]
>
> On Thu, Mar 3, 2016 at 4:19 PM, Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Oliver Neukum]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

On Sat, Mar 5, 2016 at 1:42 AM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

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
>>
>>

Re: Possible double-free in the usbnet driver

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Fri, Mar 4, 2016 at 2:26 PM, Andrey Konovalov <andreyknvl@gmail.com> 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.

I still don't think it's a double free. I think the probe thing is
called twice, and that's why to different allocations get free'd twice
(and the reason it's the same pointer is that the same allocation got
reused)

But I don't know that driver, really.

>> Which particular usbnet bind routine is this? There are multiple
>> sub-drivers for usbnet that all do different things.
>
> cdc_ncm_bind()

Ok, so that matches my theory.

Does this attached stupid patch make the warning go away? Because from
what I can tell, usbnet_link_change() really will start using that
"dev" that simply isn't valid - and will be free'd - if the bind
fails.

So you have usbnet_defer_kevent() getting triggered, which in turn
ends up using "usbnet->kevent"

But somebody like Oliver is really the right person to check this. For
example, it's entirely possible that we should just instead do

        cancel_work_sync(&dev->kevent);

before the "free_netdev(net)" in the "out1" label.

And there might be other things that bind() can have touched than the
kevent workqueue.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 851 bytes --]

 drivers/net/usb/cdc_ncm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index dc0212c3cc28..5878b54cc563 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -995,6 +995,8 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 	 * placed NDP.
 	 */
 	ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
+	if (ret < 0)
+		return ret;
 
 	/*
 	 * We should get an event when network connection is "connected" or
@@ -1003,7 +1005,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 	 * start IPv6 negotiation and more.
 	 */
 	usbnet_link_change(dev, 0, 0);
-	return ret;
+	return 0;
 }
 
 static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max)

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

On Sat, Mar 5, 2016 at 1:43 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Mar 4, 2016 at 2:26 PM, Andrey Konovalov <andreyknvl@gmail.com> 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.
>
> I still don't think it's a double free. I think the probe thing is
> called twice, and that's why to different allocations get free'd twice
> (and the reason it's the same pointer is that the same allocation got
> reused)
>
> But I don't know that driver, really.
>
>>> Which particular usbnet bind routine is this? There are multiple
>>> sub-drivers for usbnet that all do different things.
>>
>> cdc_ncm_bind()
>
> Ok, so that matches my theory.
>
> Does this attached stupid patch make the warning go away? Because from
> what I can tell, usbnet_link_change() really will start using that
> "dev" that simply isn't valid - and will be free'd - if the bind
> fails.

Yes, KASAN doesn't report anything with your patch.

>
> So you have usbnet_defer_kevent() getting triggered, which in turn
> ends up using "usbnet->kevent"
>
> But somebody like Oliver is really the right person to check this. For
> example, it's entirely possible that we should just instead do
>
>         cancel_work_sync(&dev->kevent);
>
> before the "free_netdev(net)" in the "out1" label.
>
> And there might be other things that bind() can have touched than the
> kevent workqueue.
>
>                Linus

Re: Possible double-free in the usbnet driver

[Oliver Neukum]

On Fri, 2016-03-04 at 14:43 -0800, Linus Torvalds wrote:

> So you have usbnet_defer_kevent() getting triggered, which in turn
> ends up using "usbnet->kevent"
> 
> But somebody like Oliver is really the right person to check this. For
> example, it's entirely possible that we should just instead do
> 
>         cancel_work_sync(&dev->kevent);
> 
> before the "free_netdev(net)" in the "out1" label.

Hi Bjorn,

I thinbk Linus has analyzed this correctly, but the fix really needs
to cancel the work, because we can also fail later after bind() has
already run. However, still cdc-ncm and the other drivers should clean
up after themselves if bind() fails, as usbnet really cannot known what
the subdrivers have done.

So in conclusion, I think Linus' fix should also go into cdc-ncm.

	Regards
		Oliver

Re: Possible double-free in the usbnet driver

[Bjørn Mork]

On March 5, 2016 4:51:30 PM CET, Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org> wrote:
>On Fri, 2016-03-04 at 14:43 -0800, Linus Torvalds wrote:
>
>> So you have usbnet_defer_kevent() getting triggered, which in turn
>> ends up using "usbnet->kevent"
>> 
>> But somebody like Oliver is really the right person to check this.
>For
>> example, it's entirely possible that we should just instead do
>> 
>>         cancel_work_sync(&dev->kevent);
>> 
>> before the "free_netdev(net)" in the "out1" label.
>
>Hi Bjorn,
>
>I thinbk Linus has analyzed this correctly, but the fix really needs
>to cancel the work, because we can also fail later after bind() has
>already run. However, still cdc-ncm and the other drivers should clean
>up after themselves if bind() fails, as usbnet really cannot known what
>the subdrivers have done.
>
>So in conclusion, I think Linus' fix should also go into cdc-ncm.

Definitely.  The patch is so obviously correct that we can only wonder how it was possible to miss it it the first place :)

Will take a look to see if we could do a better job cleaning up in other places.

(I do also wonder a bit about the failure to bind - is that expected or is there some bug in the cdc_ncm descriptor parsing?)


Bjørn

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Linus Torvalds]

On Sat, Mar 5, 2016 at 11:53 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>
>
> Definitely.  The patch is so obviously correct that we can only wonder how it was possible to miss it it the first place :)
>
> Will take a look to see if we could do a better job cleaning up in other places.

What should I do for 4.5? Will there be a pull request for this, or
should I just commit my cdc_ncm_bind() patch directly?

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[David Miller]

From: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Date: Mon, 7 Mar 2016 10:13:09 -0800

> On Sat, Mar 5, 2016 at 11:53 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>>
>>
>> Definitely.  The patch is so obviously correct that we can only wonder how it was possible to miss it it the first place :)
>>
>> Will take a look to see if we could do a better job cleaning up in other places.
> 
> What should I do for 4.5? Will there be a pull request for this, or
> should I just commit my cdc_ncm_bind() patch directly?

Yes I plan to send you a pull request today with Oliver's fix.

Assuming this is what you guys are referring to:

commit 1666984c8625b3db19a9abc298931d35ab7bc64b
Author: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
Date:   Mon Mar 7 11:31:10 2016 +0100

    usbnet: cleanup after bind() in probe()
    
    In case bind() works, but a later error forces bailing
    in probe() in error cases work and a timer may be scheduled.
    They must be killed. This fixes an error case related to
    the double free reported in
    http://www.spinics.net/lists/netdev/msg367669.html
    and needs to go on top of Linus' fix to cdc-ncm.
    
    Signed-off-by: Oliver Neukum <ONeukum-IBi9RG/b67k@public.gmane.org>
    Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 0b0ba7e..1079812 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1769,6 +1769,13 @@ out3:
 	if (info->unbind)
 		info->unbind (dev, udev);
 out1:
+	/* subdrivers must undo all they did in bind() if they
+	 * fail it, but we may fail later and a deferred kevent
+	 * may trigger an error resubmitting itself and, worse,
+	 * schedule a timer. So we kill it all just in case.
+	 */
+	cancel_work_sync(&dev->kevent);
+	del_timer_sync(&dev->delay);
 	free_netdev(net);
 out:
 	return status;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

On Mon, Mar 7, 2016 at 10:11 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Date: Mon, 7 Mar 2016 10:13:09 -0800
>
>> On Sat, Mar 5, 2016 at 11:53 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>>>
>>>
>>> Definitely.  The patch is so obviously correct that we can only wonder how it was possible to miss it it the first place :)
>>>
>>> Will take a look to see if we could do a better job cleaning up in other places.
>>
>> What should I do for 4.5? Will there be a pull request for this, or
>> should I just commit my cdc_ncm_bind() patch directly?
>
> Yes I plan to send you a pull request today with Oliver's fix.
>
> Assuming this is what you guys are referring to:
>
> commit 1666984c8625b3db19a9abc298931d35ab7bc64b
> Author: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
> Date:   Mon Mar 7 11:31:10 2016 +0100
>
>     usbnet: cleanup after bind() in probe()
>
>     In case bind() works, but a later error forces bailing
>     in probe() in error cases work and a timer may be scheduled.
>     They must be killed. This fixes an error case related to
>     the double free reported in
>     http://www.spinics.net/lists/netdev/msg367669.html
>     and needs to go on top of Linus' fix to cdc-ncm.
>
>     Signed-off-by: Oliver Neukum <ONeukum-IBi9RG/b67k@public.gmane.org>
>     Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

Could you also add:
Reported-by: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
?

Thanks in advance.

>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 0b0ba7e..1079812 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1769,6 +1769,13 @@ out3:
>         if (info->unbind)
>                 info->unbind (dev, udev);
>  out1:
> +       /* subdrivers must undo all they did in bind() if they
> +        * fail it, but we may fail later and a deferred kevent
> +        * may trigger an error resubmitting itself and, worse,
> +        * schedule a timer. So we kill it all just in case.
> +        */
> +       cancel_work_sync(&dev->kevent);
> +       del_timer_sync(&dev->delay);
>         free_netdev(net);
>  out:
>         return status;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[David Miller]

From: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 7 Mar 2016 22:50:41 +0300

> On Mon, Mar 7, 2016 at 10:11 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>> Date: Mon, 7 Mar 2016 10:13:09 -0800
>>
>>> On Sat, Mar 5, 2016 at 11:53 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>>>>
>>>>
>>>> Definitely.  The patch is so obviously correct that we can only wonder how it was possible to miss it it the first place :)
>>>>
>>>> Will take a look to see if we could do a better job cleaning up in other places.
>>>
>>> What should I do for 4.5? Will there be a pull request for this, or
>>> should I just commit my cdc_ncm_bind() patch directly?
>>
>> Yes I plan to send you a pull request today with Oliver's fix.
>>
>> Assuming this is what you guys are referring to:
>>
>> commit 1666984c8625b3db19a9abc298931d35ab7bc64b
>> Author: Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org>
>> Date:   Mon Mar 7 11:31:10 2016 +0100
>>
>>     usbnet: cleanup after bind() in probe()
>>
>>     In case bind() works, but a later error forces bailing
>>     in probe() in error cases work and a timer may be scheduled.
>>     They must be killed. This fixes an error case related to
>>     the double free reported in
>>     http://www.spinics.net/lists/netdev/msg367669.html
>>     and needs to go on top of Linus' fix to cdc-ncm.
>>
>>     Signed-off-by: Oliver Neukum <ONeukum-IBi9RG/b67k@public.gmane.org>
>>     Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> 
> Could you also add:
> Reported-by: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ?

Sorry it's already committed to my tree and I can't redo the commit message
once that happens since my tree has static history.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Bjørn Mork]

usbnet_link_change will call schedule_work and should be
avoided if bind is failing. Otherwise we will end up with
scheduled work referring to a netdev which has gone away.

Instead of making the call conditional, we can just defer
it to usbnet_probe, using the driver_info flag made for
this purpose.

Fixes: 8a34b0ae8778 ("usbnet: cdc_ncm: apply usbnet_link_change")
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---

David Miller <davem@davemloft.net> writes:
> From: Andrey Konovalov <andreyknvl@gmail.com>
>
>> Could you also add:
>> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
>> ?
>
> Sorry it's already committed to my tree and I can't redo the commit message
> once that happens since my tree has static history.

Even with Oliver's generic fix we should still fix the inconsistency
in cdc_ncm, as pointed out by Linus.

This is a slightly different approach than the patch proposed by Linus.
When I started looking at this I couldn't figure out why we were doing
this differently in this driver from all the other usbnet drivers
disabling the link at probe time.  So let's make it consistent.  Then at
least we get consistent bugs :)


Bjørn


 drivers/net/usb/cdc_ncm.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index be927964375b..86ba30ba35e8 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -988,8 +988,6 @@ EXPORT_SYMBOL_GPL(cdc_ncm_select_altsetting);
 
 static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 {
-	int ret;
-
 	/* MBIM backwards compatible function? */
 	if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM)
 		return -ENODEV;
@@ -998,16 +996,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 	 * Additionally, generic NCM devices are assumed to accept arbitrarily
 	 * placed NDP.
 	 */
-	ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
-
-	/*
-	 * We should get an event when network connection is "connected" or
-	 * "disconnected". Set network connection in "disconnected" state
-	 * (carrier is OFF) during attach, so the IP network stack does not
-	 * start IPv6 negotiation and more.
-	 */
-	usbnet_link_change(dev, 0, 0);
-	return ret;
+	return cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0);
 }
 
 static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max)
@@ -1590,7 +1579,8 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
 
 static const struct driver_info cdc_ncm_info = {
 	.description = "CDC NCM",
-	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET,
+	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+			| FLAG_LINK_INTR,
 	.bind = cdc_ncm_bind,
 	.unbind = cdc_ncm_unbind,
 	.manage_power = usbnet_manage_power,
@@ -1603,7 +1593,7 @@ static const struct driver_info cdc_ncm_info = {
 static const struct driver_info wwan_info = {
 	.description = "Mobile Broadband Network Device",
 	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
-			| FLAG_WWAN,
+			| FLAG_LINK_INTR | FLAG_WWAN,
 	.bind = cdc_ncm_bind,
 	.unbind = cdc_ncm_unbind,
 	.manage_power = usbnet_manage_power,
@@ -1616,7 +1606,7 @@ static const struct driver_info wwan_info = {
 static const struct driver_info wwan_noarp_info = {
 	.description = "Mobile Broadband Network Device (NO ARP)",
 	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
-			| FLAG_WWAN | FLAG_NOARP,
+			| FLAG_LINK_INTR | FLAG_WWAN | FLAG_NOARP,
 	.bind = cdc_ncm_bind,
 	.unbind = cdc_ncm_unbind,
 	.manage_power = usbnet_manage_power,
-- 
2.1.4

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[David Miller]

From: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
Date: Mon, 7 Mar 2016 21:15:36 +0100

> usbnet_link_change will call schedule_work and should be
> avoided if bind is failing. Otherwise we will end up with
> scheduled work referring to a netdev which has gone away.
> 
> Instead of making the call conditional, we can just defer
> it to usbnet_probe, using the driver_info flag made for
> this purpose.
> 
> Fixes: 8a34b0ae8778 ("usbnet: cdc_ncm: apply usbnet_link_change")
> Reported-by: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Suggested-by: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
...
> Even with Oliver's generic fix we should still fix the inconsistency
> in cdc_ncm, as pointed out by Linus.
> 
> This is a slightly different approach than the patch proposed by Linus.
> When I started looking at this I couldn't figure out why we were doing
> this differently in this driver from all the other usbnet drivers
> disabling the link at probe time.  So let's make it consistent.  Then at
> least we get consistent bugs :)

Fair enough, applied and queued up for -stable.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Linus Torvalds]

On Mon, Mar 7, 2016 at 12:15 PM, Bjørn Mork <bjorn@mork.no> wrote:
> usbnet_link_change will call schedule_work and should be
> avoided if bind is failing. Otherwise we will end up with
> scheduled work referring to a netdev which has gone away.
>
> Instead of making the call conditional, we can just defer
> it to usbnet_probe, using the driver_info flag made for
> this purpose.

So looking at this, I wonder...

Why is that FLAG_LINK_INTR thing not just always used?

The _only_ thing that FLAG_LINK_INTR does is to cause

        usbnet_link_change(dev, 0, 0);

to be called after network device attach. That doesn't seem to be controversial.

Looking at some examples, we have ax88179_178a.c that doesn't set the
flag, but instead does that usbnet_link_change() call at the end of
ax88179_bind().

There are a few drivers that seem to never call that
usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
Would they break?

Stupid grep:

    git grep -lw FLAG_ETHER |
        xargs grep -L FLAG_LINK_INTR |
        xargs grep -L usbnet_link_change |
        sed 's:drivers/net/usb/::'

gives

    cdc_eem.c
    ch9200.c
    cx82310_eth.c
    int51x1.c
    rndis_host.c

so maybe that FLAG_LINK_INTR si required.

Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
anything "INTR" about it.

               Linus

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Oliver Neukum]

On Tue, 2016-03-08 at 11:43 -0800, Linus Torvalds wrote:
> Why is that FLAG_LINK_INTR thing not just always used?
> 
> The _only_ thing that FLAG_LINK_INTR does is to cause
> 
>         usbnet_link_change(dev, 0, 0);
> 
> to be called after network device attach. That doesn't seem to be
> controversial.

It depends on the devices' capabilities.
For a few old devices that would be deadly, because they cannot
indicate that a link goes up again.

> Looking at some examples, we have ax88179_178a.c that doesn't set the
> flag, but instead does that usbnet_link_change() call at the end of
> ax88179_bind().
> 
> There are a few drivers that seem to never call that
> usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> Would they break?

Yes. If we did the call unconditionally. We could not do it,
then we'd see some spurious detection of interfaces being up,
but something breaks. Today I'd probably require a flag
for those cases that do not want the call to be made, but the
distinction as such is necessary.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Bjørn Mork]

Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> writes:

> So looking at this, I wonder...
>
> Why is that FLAG_LINK_INTR thing not just always used?
>
> The _only_ thing that FLAG_LINK_INTR does is to cause
>
>         usbnet_link_change(dev, 0, 0);
>
> to be called after network device attach. That doesn't seem to be controversial.

Not all usbnet drivers support carrier detection, which is required to
ever bring the link up again.

> Looking at some examples, we have ax88179_178a.c that doesn't set the
> flag, but instead does that usbnet_link_change() call at the end of
> ax88179_bind().
>
> There are a few drivers that seem to never call that
> usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> Would they break?

Yes.  Drivers without carrier detection will be "down" forever.

> Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> anything "INTR" about it.

Beats me.  I can only say that I always find naming difficult...
We could ask Ben, who introduced it in:


commit 37e8273cd30592d3a82bcb70cbb1bdc4eaeb6b71
Author: Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
Date:   Wed Nov 4 15:29:52 2009 +0000

    usbnet: Set link down initially for drivers that update link state
    
    Some usbnet drivers update link state while others do not due to
    hardware limitations.  Add a flag to distinguish those that do, and
    set the link down initially for their devices.
    
    This is intended to fix this bug: http://bugs.debian.org/444043
    
    Signed-off-by: Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>
    Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>



But I guess it doesn't really matter.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Oliver Neukum]

On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote:
> > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> > anything "INTR" about it.
> 
> Beats me.  I can only say that I always find naming difficult...
> We could ask Ben, who introduced it in:

It used to be done over USB interrupt endpoints.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cdc_ncm: do not call usbnet_link_change from cdc_ncm_bind

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > 
> > So looking at this, I wonder...
> > 
> > Why is that FLAG_LINK_INTR thing not just always used?
> > 
> > The _only_ thing that FLAG_LINK_INTR does is to cause
> > 
> >         usbnet_link_change(dev, 0, 0);
> > 
> > to be called after network device attach. That doesn't seem to be controversial.
> Not all usbnet drivers support carrier detection, which is required to
> ever bring the link up again.
> 
> > 
> > Looking at some examples, we have ax88179_178a.c that doesn't set the
> > flag, but instead does that usbnet_link_change() call at the end of
> > ax88179_bind().
> > 
> > There are a few drivers that seem to never call that
> > usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag.
> > Would they break?
> Yes.  Drivers without carrier detection will be "down" forever.
> 
> > 
> > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be
> > anything "INTR" about it.
> Beats me.  I can only say that I always find naming difficult...
> We could ask Ben, who introduced it in:
[...]

It is supposed to imply that the device generates link-change
interrupts.  Of course it is also possible for a device driver to
satisfy the requirement by polling the link state.

Ben.

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Possible double-free in the usbnet driver

[Oliver Neukum]

On Mon, 2016-03-07 at 22:50 +0300, Andrey Konovalov wrote:
> Could you also add:
> Reported-by: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Well, the exact bug you reported is fixed in Bjorn's
patch the way Linus suggested. I'm fixing just a further
race that would require an error condition on top
of what you have seen.
So I don't think your Reported-by would be totally
appropriate.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Andrey Konovalov]

On Tue, Mar 8, 2016 at 12:39 AM, Oliver Neukum <oneukum-IBi9RG/b67k@public.gmane.org> wrote:
> On Mon, 2016-03-07 at 22:50 +0300, Andrey Konovalov wrote:
>> Could you also add:
>> Reported-by: Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Well, the exact bug you reported is fixed in Bjorn's
> patch the way Linus suggested. I'm fixing just a further
> race that would require an error condition on top
> of what you have seen.
> So I don't think your Reported-by would be totally
> appropriate.

Oh, OK, Sorry. I thought this was a part of the same fix.

>
>         Regards
>                 Oliver
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible double-free in the usbnet driver

[Dmitry Vyukov]

On Fri, Mar 4, 2016 at 11:43 PM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> On Fri, Mar 4, 2016 at 2:26 PM, Andrey Konovalov <andreyknvl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
>
> I still don't think it's a double free. I think the probe thing is
> called twice, and that's why to different allocations get free'd twice
> (and the reason it's the same pointer is that the same allocation got
> reused)


FYI, we have a KASAN patch in flight that adds "quarantine" for freed
memory (memory is reused only after a significant delay). It should
help to easily differentiate between use-after-free, double-free and
heap-out-of-bound. Yes, now it is confusing.


> But I don't know that driver, really.
>
>>> Which particular usbnet bind routine is this? There are multiple
>>> sub-drivers for usbnet that all do different things.
>>
>> cdc_ncm_bind()
>
> Ok, so that matches my theory.
>
> Does this attached stupid patch make the warning go away? Because from
> what I can tell, usbnet_link_change() really will start using that
> "dev" that simply isn't valid - and will be free'd - if the bind
> fails.
>
> So you have usbnet_defer_kevent() getting triggered, which in turn
> ends up using "usbnet->kevent"
>
> But somebody like Oliver is really the right person to check this. For
> example, it's entirely possible that we should just instead do
>
>         cancel_work_sync(&dev->kevent);
>
> before the "free_netdev(net)" in the "out1" label.
>
> And there might be other things that bind() can have touched than the
> kevent workqueue.
>
>                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html