[PATCH] usb/core: fix NULL pointer dereference in recursively_mark_NOTATTACHED

[Du, ChangbinX]

From ee48b3736d437af79d4fe858cdf64f241c76c339 Mon Sep 17 00:00:00 2001
From: "Du, Changbin" <changb...@intel.com>
Date: Wed, 18 Dec 2013 16:47:21 +0800
Subject: [PATCH] usb/core: fix NULL pointer dereference in
recursively_mark_NOTATTACHED

usb_hub_to_struct_hub() can return NULL if the hub without active
configuration. So the result must be checked.

BUG: unable to handle kernel NULL pointer dereference at 0000015c
IP: [<c16d5fb0>] recursively_mark_NOTATTACHED+0x20/0x60
Call Trace:
[<c16d5fc4>] recursively_mark_NOTATTACHED+0x34/0x60
[<c16d5fc4>] recursively_mark_NOTATTACHED+0x34/0x60
[<c16d5fc4>] recursively_mark_NOTATTACHED+0x34/0x60
[<c16d5fc4>] recursively_mark_NOTATTACHED+0x34/0x60
[<c16d6082>] usb_set_device_state+0x92/0x120
[<c16d862b>] usb_disconnect+0x2b/0x1a0
[<c16dd4c0>] usb_remove_hcd+0xb0/0x160
[<c19ca846>] ? _raw_spin_unlock_irqrestore+0x26/0x50
[<c1704efc>] ehci_mid_remove+0x1c/0x30
[<c1704f26>] ehci_mid_stop_host+0x16/0x30
[<c16f7698>] penwell_otg_work+0xd28/0x3520
[<c19c945b>] ? __schedule+0x39b/0x7f0
[<c19cdb9d>] ? sub_preempt_count+0x3d/0x50
[<c125e97d>] process_one_work+0x11d/0x3d0
[<c19c7f4d>] ? mutex_unlock+0xd/0x10
[<c125e0e5>] ? manage_workers.isra.24+0x1b5/0x270
[<c125f009>] worker_thread+0xf9/0x320
[<c19ca846>] ? _raw_spin_unlock_irqrestore+0x26/0x50
[<c125ef10>] ? rescuer_thread+0x2b0/0x2b0
[<c1264ac4>] kthread+0x94/0xa0
[<c19d0f77>] ret_from_kernel_thread+0x1b/0x28
[<c1264a30>] ? kthread_create_on_node+0xc0/0xc0

Signed-off-by: Du, Changbin <changb...@intel.com>
---
drivers/usb/core/hub.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bd9dc35..01a1fe6 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1870,9 +1870,11 @@ static void recursively_mark_NOTATTACHED(struct usb_device *udev)
struct usb_hub *hub = usb_hub_to_struct_hub(udev);
int i;

- for (i = 0; i < udev->maxchild; ++i) {
- if (hub->ports[i]->child)
- recursively_mark_NOTATTACHED(hub->ports[i]->child);
+ if (hub) {
+ for (i = 0; i < udev->maxchild; ++i) {
+ if (hub->ports[i]->child)
+ recursively_mark_NOTATTACHED(hub->ports[i]->child);
+ }
}
if (udev->state == USB_STATE_SUSPENDED)
udev->active_duration -= jiffies;
--
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Alan Stern]

How did you manage to trigger this BUG? If hub is NULL then
udev->maxchild should be 0. See the code in hub_disconnect().

Alan Stern

[Du, ChangbinX]

> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: Monday, December 23, 2013 11:13 PM
> To: Du, ChangbinX
> Cc: gre...@linuxfoundation.org; sarah....@linux.intel.com; Lan, Tianyu;
> burza...@gmail.com; linu...@vger.kernel.org;
> linux-...@vger.kernel.org
> Subject: Re: [PATCH] usb/core: fix NULL pointer dereference in
> recursively_mark_NOTATTACHED

>
> On Mon, 23 Dec 2013, Du, ChangbinX wrote:
>
> > usb_hub_to_struct_hub() can return NULL if the hub without active
> > configuration. So the result must be checked.
> >
> > BUG: unable to handle kernel NULL pointer dereference at 0000015c

> How did you manage to trigger this BUG? If hub is NULL then
> udev->maxchild should be 0. See the code in hub_disconnect().
>
> Alan Stern

Hello, Alan. The hub also should be null if actconfig is null. You can see it in function
usb_hub_to_struct_hub().
udev->maxchild will be set to 0 in hub_disconnect(). But before that,
recursively_mark_NOTATTACHED may be called when calling usb_disconnect(). So this issue
will happen when usb_disconnect a hub that not have a configuration yet.
It happened once here when unplugging otg cable from DUT(will cause hcd removed) with
tiers of hub and devices. But it's not easy to reproduce it.
This is my analysis, how do you think?

Du, Changbin

[Alan Stern]

On Tue, 24 Dec 2013, Du, ChangbinX wrote:

> > > usb_hub_to_struct_hub() can return NULL if the hub without active
> > > configuration. So the result must be checked.
> > >
> > > BUG: unable to handle kernel NULL pointer dereference at 0000015c
>
> > How did you manage to trigger this BUG? If hub is NULL then
> > udev->maxchild should be 0. See the code in hub_disconnect().
> >
> > Alan Stern
>
> Hello, Alan. The hub also should be null if actconfig is null. You can see it in function
> usb_hub_to_struct_hub().

Yes, I know. But if actconfig is NULL then udev->maxchild should be 0.

> udev->maxchild will be set to 0 in hub_disconnect().

Note that hub_disconnect() runs _before_ actconfig is set to NULL.

> But before that,
> recursively_mark_NOTATTACHED may be called when calling usb_disconnect().

If this happens before hub_disconnect() has run then actconfig cannot
be NULL, because hub_disconnect() runs _before_ actconfig is set to
NULL.

> So this issue
> will happen when usb_disconnect a hub that not have a configuration yet.

No, it won't. You can test this easily. Plug in a hub, write 0 to its
/sys/bus/usb/devices/.../bConfigurationValue, and then unplug the hub.

> It happened once here when unplugging otg cable from DUT(will cause hcd removed) with
> tiers of hub and devices. But it's not easy to reproduce it.
> This is my analysis, how do you think?

There is another reason why usb_hub_to_struct_hub() could return NULL:
if usb_get_intfdata(hdev->actconfig->interface[0]) is NULL. This could
happen if recursively_mark_NOTATTACHED() is called _while_
hub_disconnect() is running.

There should be locking to prevent this, but there isn't. That's what
we need to fix. It's not an easy problem. Can you figure out a
correct solution?

Alan Stern

[Alan Stern]

On Tue, 24 Dec 2013, Alan Stern wrote:

> > This is my analysis, how do you think?
>
> There is another reason why usb_hub_to_struct_hub() could return NULL:
> if usb_get_intfdata(hdev->actconfig->interface[0]) is NULL. This could
> happen if recursively_mark_NOTATTACHED() is called _while_
> hub_disconnect() is running.
>
> There should be locking to prevent this, but there isn't. That's what
> we need to fix. It's not an easy problem. Can you figure out a
> correct solution?

I think this will fix it. Take a close look and do some careful
testing.

Alan Stern



Index: usb-3.13/drivers/usb/core/hub.c
===================================================================
--- usb-3.13.orig/drivers/usb/core/hub.c
+++ usb-3.13/drivers/usb/core/hub.c
@@ -1622,11 +1622,14 @@ static void hub_disconnect(struct usb_in
hub->error = 0;
hub_quiesce(hub, HUB_DISCONNECT);

- usb_set_intfdata (intf, NULL);
-
for (i = 0; i < hdev->maxchild; i++)
usb_hub_remove_port_device(hub, i + 1);
+
+ /* Avoid races with recursively_mark_NOTATTACHED() */
+ spin_lock_irq(&device_state_lock);
hub->hdev->maxchild = 0;
+ usb_set_intfdata(intf, NULL);
+ spin_unlock_irq(&device_state_lock);

if (hub->hdev->speed == USB_SPEED_HIGH)
highspeed_hubs--;

[Du, ChangbinX]

> On Tue, 24 Dec 2013, Alan Stern wrote:
> I think this will fix it. Take a close look and do some careful testing.
>
> Alan Stern
>
> Index: usb-3.13/drivers/usb/core/hub.c
> ==========================================================
> =========
> --- usb-3.13.orig/drivers/usb/core/hub.c
> +++ usb-3.13/drivers/usb/core/hub.c
> @@ -1622,11 +1622,14 @@ static void hub_disconnect(struct usb_in
> hub->error = 0;
> hub_quiesce(hub, HUB_DISCONNECT);
>
> - usb_set_intfdata (intf, NULL);
> -
> for (i = 0; i < hdev->maxchild; i++)
> usb_hub_remove_port_device(hub, i + 1);
> +
> + /* Avoid races with recursively_mark_NOTATTACHED() */
> + spin_lock_irq(&device_state_lock);
> hub->hdev->maxchild = 0;
> + usb_set_intfdata(intf, NULL);
> + spin_unlock_irq(&device_state_lock);
>
> if (hub->hdev->speed == USB_SPEED_HIGH)
> highspeed_hubs--;
>

Sorry for late response. Agree with you. I will test your patch carefully and
let you know the result. Thanks!

[Du, ChangbinX]

> > On Tue, 24 Dec 2013, Alan Stern wrote:
> > I think this will fix it. Take a close look and do some careful testing.
> >
> > Alan Stern
> >
> > Index: usb-3.13/drivers/usb/core/hub.c
> >
> ==========================================================

> > --- usb-3.13.orig/drivers/usb/core/hub.c
> > +++ usb-3.13/drivers/usb/core/hub.c
> > @@ -1622,11 +1622,14 @@ static void hub_disconnect(struct usb_in
> > hub->error = 0;
> > hub_quiesce(hub, HUB_DISCONNECT);
> >
> > - usb_set_intfdata (intf, NULL);
> > -
> > for (i = 0; i < hdev->maxchild; i++)
> > usb_hub_remove_port_device(hub, i + 1);
> > +
> > + /* Avoid races with recursively_mark_NOTATTACHED() */
> > + spin_lock_irq(&device_state_lock);
> > hub->hdev->maxchild = 0;
> > + usb_set_intfdata(intf, NULL);
> > + spin_unlock_irq(&device_state_lock);
> >
> > if (hub->hdev->speed == USB_SPEED_HIGH)
> > highspeed_hubs--;
> >
>

> Sorry for late response. Agree with you. I will test your patch carefully and
> let you know the result. Thanks!

I can reproduce issue by adding a delay just after usb_set_intfdata(intf, NULL)
(echo -1 > bConfigurationValue to trigger hub_dissconnect())without your patch.

After patch applied, cannot reproduce and didn't found any other issue. Patch works well.

Alan, need I update patch to v2 or you will do it?

Du, Changbin

[Alan Stern]

On Thu, 26 Dec 2013, Du, ChangbinX wrote:

> I can reproduce issue by adding a delay just after usb_set_intfdata(intf, NULL)
> (echo -1 > bConfigurationValue to trigger hub_dissconnect())without your patch.
>
> After patch applied, cannot reproduce and didn't found any other issue. Patch works well.
>
> Alan, need I update patch to v2 or you will do it?

I will send in the updated patch next week, after I get back from
vacation.

Alan Stern

[Alan Stern]

On Thu, 26 Dec 2013, Du, ChangbinX wrote:

> I can reproduce issue by adding a delay just after usb_set_intfdata(intf, NULL)
> (echo -1 > bConfigurationValue to trigger hub_dissconnect())without your patch.
>
> After patch applied, cannot reproduce and didn't found any other issue. Patch works well.
>
> Alan, need I update patch to v2 or you will do it?

Changbin, after looking more closely I realized there was a second
aspect to this race: recursively_mark_NOTATTACHED uses hub->ports[i]
while hub_disconnect removes the port devices. You ought to be
able to cause an oops by inserting a delay just after the loop where
usb_hub_remove_port_device is called.

The updated patch below should fix both problems. Can you test it?

Alan Stern



Index: usb-3.13/drivers/usb/core/hub.c
===================================================================

--- usb-3.13.orig/drivers/usb/core/hub.c
+++ usb-3.13/drivers/usb/core/hub.c
@@ -1607,7 +1607,7 @@ static void hub_disconnect(struct usb_in
{
struct usb_hub *hub = usb_get_intfdata(intf);
struct usb_device *hdev = interface_to_usbdev(intf);
- int i;
+ int port1;

/* Take the hub off the event list and don't let it be added again */
spin_lock_irq(&hub_event_lock);
@@ -1622,11 +1622,15 @@ static void hub_disconnect(struct usb_in

hub->error = 0;
hub_quiesce(hub, HUB_DISCONNECT);

- usb_set_intfdata (intf, NULL);

+ /* Avoid races with recursively_mark_NOTATTACHED() */
+ spin_lock_irq(&device_state_lock);

+ port1 = hdev->maxchild;
+ hdev->maxchild = 0;

+ usb_set_intfdata(intf, NULL);
+ spin_unlock_irq(&device_state_lock);

- for (i = 0; i < hdev->maxchild; i++)

- usb_hub_remove_port_device(hub, i + 1);
- hub->hdev->maxchild = 0;
+ for (; port1 > 0; --port1)
+ usb_hub_remove_port_device(hub, port1);

if (hub->hdev->speed == USB_SPEED_HIGH)
highspeed_hubs--;

[Du, ChangbinX]

> On Thu, 26 Dec 2013, Du, ChangbinX wrote:
>
> > I can reproduce issue by adding a delay just after
> > usb_set_intfdata(intf, NULL) (echo -1 > bConfigurationValue to trigger
> hub_dissconnect())without your patch.
> >
> > After patch applied, cannot reproduce and didn't found any other issue.
> Patch works well.
> >
> > Alan, need I update patch to v2 or you will do it?
>
> Changbin, after looking more closely I realized there was a second aspect to
> this race: recursively_mark_NOTATTACHED uses hub->ports[i] while
> hub_disconnect removes the port devices. You ought to be able to cause
> an oops by inserting a delay just after the loop where
> usb_hub_remove_port_device is called.
>
> The updated patch below should fix both problems. Can you test it?
>
> Alan Stern
>

Ok, I'll test it today or tomorrow. Please wait my response.

[Du, ChangbinX]

> > Changbin, after looking more closely I realized there was a second
> > aspect to this race: recursively_mark_NOTATTACHED uses hub->ports[i]
> > while hub_disconnect removes the port devices. You ought to be able
> > to cause an oops by inserting a delay just after the loop where
> > usb_hub_remove_port_device is called.
> >
> > The updated patch below should fix both problems. Can you test it?
> >
> > Alan Stern
> >
>
> Ok, I'll test it today or tomorrow. Please wait my response.

Alan, I cannot cause a panic after inserting a delay just after
usb_hub_remove_port_device is called, even move the delay after
kfree(hub->ports). recursively_mark_NOTATTACHED will not access
hub->ports[i] since maxchild has been set to 0.

Alan, I think your last patch can fix this issue.

[Alan Stern]

On Tue, 7 Jan 2014, Du, ChangbinX wrote:

> > > Changbin, after looking more closely I realized there was a second
> > > aspect to this race: recursively_mark_NOTATTACHED uses hub->ports[i]
> > > while hub_disconnect removes the port devices. You ought to be able
> > > to cause an oops by inserting a delay just after the loop where
> > > usb_hub_remove_port_device is called.
> > >
> > > The updated patch below should fix both problems. Can you test it?
> > >
> > > Alan Stern
> > >
> >
> > Ok, I'll test it today or tomorrow. Please wait my response.
>
> Alan, I cannot cause a panic after inserting a delay just after
> usb_hub_remove_port_device is called, even move the delay after
> kfree(hub->ports). recursively_mark_NOTATTACHED will not access
> hub->ports[i] since maxchild has been set to 0.
>
> Alan, I think your last patch can fix this issue.

Okay, thanks for testing. I will submit the patch.

Alan Stern