Re: Possible double-free in the usbnet driver

From: Andrey Konovalov <redacted>
Date: Mon, 7 Mar 2016 22:50:41 +0300

On Mon, Mar 7, 2016 at 10:11 PM, David Miller [off-list ref] wrote:

quoted

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

quoted

On Sat, Mar 5, 2016 at 11:53 AM, Bjørn Mork [off-list ref] wrote:

quoted

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 [off-list ref]
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 [off-list ref]
    Signed-off-by: David S. Miller [off-list ref]

Could you also add:
Reported-by: Andrey Konovalov <redacted>
?

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

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 [off-list ref] writes:

From: Andrey Konovalov <andreyknvl@gmail.com>

quoted

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

On Mon, Mar 7, 2016 at 12:15 PM, Bjørn Mork [off-list ref] 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