[U-Boot] usb: hub enumeration and potential NULL ptr dereference in usb_device_info()

[U-Boot] usb: hub enumeration and potential NULL ptr dereference in usb_device_info()

[Bernhard Nortmann]

Starting with commit b19236fd1c1ef289bab9e243ee5b50d658fcac3f I am observing
a breakage of the "usb info" command on my BananaPi (Allwinner A20, sun7i),
while "usb tree" and dm commands ("dm tree", "dm uclass") are fine.
See attached usb-info-breakage.log

Tracing back the error positon from pc and lr registers pointed me to the
device_get_uclass_id() call within usb_device_info(), and suggested the
problem is caused by trying to dereference a NULL struct udevice *hub.

Therefore I added a diagnostic printf and a safeguard to this routine:

diff --git a/cmd/usb.c b/cmd/usb.c
index b83d323..a5e2463 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -610,6 +610,12 @@ static int usb_device_info(void)
          struct udevice *hub;

          device_find_first_child(bus, &hub);
+        printf("bus %p, uclass %d, hub %p: ",
+            bus, device_get_uclass_id(bus), hub);
+        if (!hub) {
+            printf("<no hub>\n");
+            continue;
+        }
          if (device_get_uclass_id(hub) == UCLASS_USB_HUB &&
              device_active(hub)) {
              show_info(hub);

And it became apparent that hub actually receives NULL values during the
device enumeration. The safeguard prevented the "data abort" error and got
"usb info" working again - see attached usb-info-fixed.log

I'm not sure why this particular problem didn't manifest earlier and
only now became apparent with the change in SPL header size /
CONFIG_SPL_TEXT_BASE. But it seems that accessing a NULL hub with
device_get_uclass_id() is clearly wrong and should be avoided.
Which brings me to two questions:

1. Is getting a NULL hub value legit when iterating UCLASS_USB devices
    the way that usb_device_info() does? If yes, the code seems to be
    lacking protection against passing it to device_get_uclass_id().

2. Why does usb_device_info() choose to enumerate hubs this way at all?
    If the routine is aiming at UCLASS_USB_HUB - which seems to be the
    purpose of the subsequent device_get_uclass_id(hub) test - and the
    device tree already provides this information (as suggested by the
    output of "dm uclass"), then why not enumerate UCLASS_USB_HUB directly?

Regards, B. Nortmann

-------------- next part --------------
A non-text attachment was scrubbed...
Name: usb-info-breakage.log
Type: application/octet-stream
Size: 3004 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160622/02f353a1/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: usb-info-fixed.log
Type: application/octet-stream
Size: 3166 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160622/02f353a1/attachment-0001.obj>

[U-Boot] usb: hub enumeration and potential NULL ptr dereference in usb_device_info()

[Simon Glass]

Hi Benhard,

On 22 June 2016 at 03:05, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> Starting with commit b19236fd1c1ef289bab9e243ee5b50d658fcac3f I am observing
> a breakage of the "usb info" command on my BananaPi (Allwinner A20, sun7i),
> while "usb tree" and dm commands ("dm tree", "dm uclass") are fine.
> See attached usb-info-breakage.log
>
> Tracing back the error positon from pc and lr registers pointed me to the
> device_get_uclass_id() call within usb_device_info(), and suggested the
> problem is caused by trying to dereference a NULL struct udevice *hub.
>
> Therefore I added a diagnostic printf and a safeguard to this routine:
>
> diff --git a/cmd/usb.c b/cmd/usb.c
> index b83d323..a5e2463 100644
> --- a/cmd/usb.c
> +++ b/cmd/usb.c
> @@ -610,6 +610,12 @@ static int usb_device_info(void)
>          struct udevice *hub;
>
>          device_find_first_child(bus, &hub);
> +        printf("bus %p, uclass %d, hub %p: ",
> +            bus, device_get_uclass_id(bus), hub);
> +        if (!hub) {
> +            printf("<no hub>\n");
> +            continue;
> +        }
>          if (device_get_uclass_id(hub) == UCLASS_USB_HUB &&
>              device_active(hub)) {
>              show_info(hub);
>
> And it became apparent that hub actually receives NULL values during the
> device enumeration. The safeguard prevented the "data abort" error and got
> "usb info" working again - see attached usb-info-fixed.log
>
> I'm not sure why this particular problem didn't manifest earlier and
> only now became apparent with the change in SPL header size /
> CONFIG_SPL_TEXT_BASE. But it seems that accessing a NULL hub with
> device_get_uclass_id() is clearly wrong and should be avoided.
> Which brings me to two questions:
>
> 1. Is getting a NULL hub value legit when iterating UCLASS_USB devices
>    the way that usb_device_info() does? If yes, the code seems to be
>    lacking protection against passing it to device_get_uclass_id().

Well if there are no child devices of a USB controller, then yes.
Normally there is at least one (a USB hub). But this code is wrong -
it should not assume that.

In fact. now, the new uclass_first_device_err() function is probably a
better choice, since it returns an error if nothing is found.

>
> 2. Why does usb_device_info() choose to enumerate hubs this way at all?
>    If the routine is aiming at UCLASS_USB_HUB - which seems to be the
>    purpose of the subsequent device_get_uclass_id(hub) test - and the
>    device tree already provides this information (as suggested by the
>    output of "dm uclass"), then why not enumerate UCLASS_USB_HUB directly?

It was probably trying to duplicate the operation of the old code:

if (strncmp(argv[1], "inf", 3) == 0) {
if (argc == 2) {
#ifdef CONFIG_DM_USB
usb_device_info();
#else
int d;
for (d = 0; d < USB_MAX_DEVICE; d++) {
udev = usb_get_dev_index(d);
if (udev == NULL)
break;
usb_display_desc(udev);
usb_display_config(udev);
}
#endif

But I'm not sure that the ordering would change in any case, or even
if it matters. Feel free to change it to enumerate USB_HUB.

Another explanation is that originally I was not sure if USB hubs
should have their own uclass. With PCI bridges we don't do it that
way. But I decided in the end to go ahead, and I think it has worked
ouit. So perhaps the code was converted mid-stream.

>
> Regards, B. Nortmann
>

Regards,
Simon

[U-Boot] usb: hub enumeration and potential NULL ptr dereference in usb_device_info()

[Bernhard Nortmann]

Hi Simon, thanks for replying!

Am 23.06.2016 um 15:12 schrieb Simon Glass:
> Hi Benhard,
>
> On 22 June 2016 at 03:05, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
>> [...]
>>
>> I'm not sure why this particular problem didn't manifest earlier and
>> only now became apparent with the change in SPL header size /
>> CONFIG_SPL_TEXT_BASE. But it seems that accessing a NULL hub with
>> device_get_uclass_id() is clearly wrong and should be avoided.
>> Which brings me to two questions:
>>
>> 1. Is getting a NULL hub value legit when iterating UCLASS_USB devices
>>     the way that usb_device_info() does? If yes, the code seems to be
>>     lacking protection against passing it to device_get_uclass_id().
> Well if there are no child devices of a USB controller, then yes.
> Normally there is at least one (a USB hub). But this code is wrong -
> it should not assume that.
>
> In fact. now, the new uclass_first_device_err() function is probably a
> better choice, since it returns an error if nothing is found.

uclass_first_device_err() won't help here. It's not the outer UCLASS_USB
enumeration that is at fault - getting a NULL "bus" would simply terminate
the loop. In fact this loop correctly processes all four 'top level'
devices available on sun7i-a20:

uclass 60: usb
- * usb at 01c14000 @ 7af37148, seq 0, (req -1)
- * usb at 01c14400 @ 7af371b0, seq 1, (req -1)
- * usb at 01c1c000 @ 7af37218, seq 2, (req -1)
- * usb at 01c1c400 @ 7af37280, seq 3, (req -1)

The root cause of the usb_device_info() problem here is that two of these
controllers (ohci0: usb at 01c14400, ohci1: usb at 01c1c400) remain dormant /
'invisible' to DM as long as no actual USB1-only peripheral is attached, and
device_find_first_child(bus, &hub) will return a NULL hub subsequently.
("usb tree" doesn't show them either in this state.)

I have a suspicion that this might easily happen with other EHCI-OHCI
controller combinations too.

>
>> 2. Why does usb_device_info() choose to enumerate hubs this way at all?
>>     If the routine is aiming at UCLASS_USB_HUB - which seems to be the
>>     purpose of the subsequent device_get_uclass_id(hub) test - and the
>>     device tree already provides this information (as suggested by the
>>     output of "dm uclass"), then why not enumerate UCLASS_USB_HUB directly?
> It was probably trying to duplicate the operation of the old code:
> [...]
>
> But I'm not sure that the ordering would change in any case, or even
> if it matters. Feel free to change it to enumerate USB_HUB.
>
> Another explanation is that originally I was not sure if USB hubs
> should have their own uclass. With PCI bridges we don't do it that
> way. But I decided in the end to go ahead, and I think it has worked
> ouit. So perhaps the code was converted mid-stream.

Yes, I figured it might be something along those "historical" lines. But if
I understand you correctly, then any "usb_hub" (uclass 62) child of 
UCLASS_USB
should also show up when iterating UCLASS_USB_HUB with uclass_first_device()
and uclass_next_device(). If that's guaranteed, my preferred solution would
actually be to do away with the "bus" enumeration and replace it with a more
straightforward "show_info(hub) over all the hubs" solution.

I'll submit a patch for that shortly.

Regards, B. Nortmann

[U-Boot] [PATCH v1] dm: usb: Prevent NULL hub in usb_device_info()

[Bernhard Nortmann]

This patch modifies the usb_device_info() function to enumerate
active USB hubs by iterating UCLASS_USB_HUB directly.

The previous code used UCLASS_USB nodes instead and retrieved
their first child node, expecting it to be a hub. However, it
did not protect against retrieving (and dereferencing) a NULL
pointer this way.

Depending on the available USB hardware, this might happen easily.
For example the USB controller on sun7i-a20 has top-level OHCI
nodes that won't have any children as long as no USB1-only
peripheral is attached. ("usb tree" will only show EHCI nodes.)

The failure can also be demonstrated with U-Boot's sandbox
architecture, by simply enabling the inactive "usb_2" node in
test.dts. This creates a similar situation, where the existing
usb_device_info() implementation would crash (segfault) when
issuing a "usb info" command.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>

---

 cmd/usb.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/cmd/usb.c b/cmd/usb.c
index 58d9db2..3146f54 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -602,18 +602,13 @@ static void show_info(struct udevice *dev)
 
 static int usb_device_info(void)
 {
-	struct udevice *bus;
-
-	for (uclass_first_device(UCLASS_USB, &bus);
-	     bus;
-	     uclass_next_device(&bus)) {
-		struct udevice *hub;
+	struct udevice *hub;
 
-		device_find_first_child(bus, &hub);
-		if (device_get_uclass_id(hub) == UCLASS_USB_HUB &&
-		    device_active(hub)) {
+	for (uclass_first_device(UCLASS_USB_HUB, &hub);
+	     hub;
+	     uclass_next_device(&hub)) {
+		if (device_active(hub))
 			show_info(hub);
-		}
 	}
 
 	return 0;
-- 
2.7.3

[U-Boot] [PATCH v1] dm: usb: Prevent NULL hub in usb_device_info()

[Simon Glass]

Hi Bernhard,

On 29 June 2016 at 01:43, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> This patch modifies the usb_device_info() function to enumerate
> active USB hubs by iterating UCLASS_USB_HUB directly.
>
> The previous code used UCLASS_USB nodes instead and retrieved
> their first child node, expecting it to be a hub. However, it
> did not protect against retrieving (and dereferencing) a NULL
> pointer this way.
>
> Depending on the available USB hardware, this might happen easily.
> For example the USB controller on sun7i-a20 has top-level OHCI
> nodes that won't have any children as long as no USB1-only
> peripheral is attached. ("usb tree" will only show EHCI nodes.)
>
> The failure can also be demonstrated with U-Boot's sandbox
> architecture, by simply enabling the inactive "usb_2" node in
> test.dts. This creates a similar situation, where the existing
> usb_device_info() implementation would crash (segfault) when
> issuing a "usb info" command.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
>
> ---
>
>  cmd/usb.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

My only concern is whether this changes the ordering (since the
devices' devnum is printed out). But the existing ordering may not be
any better anyway, and if we had a problem we could sort it before
outputting the info, so it seems OK to me.

Thanks for fixing the bug.

- Simon

[U-Boot] [U-Boot, v1] dm: usb: Prevent NULL hub in usb_device_info()

[Hans de Goede]

Hi,

On 29-06-16 10:43, Bernhard Nortmann wrote:
> This patch modifies the usb_device_info() function to enumerate
> active USB hubs by iterating UCLASS_USB_HUB directly.
>
> The previous code used UCLASS_USB nodes instead and retrieved
> their first child node, expecting it to be a hub. However, it
> did not protect against retrieving (and dereferencing) a NULL
> pointer this way.
>
> Depending on the available USB hardware, this might happen easily.
> For example the USB controller on sun7i-a20 has top-level OHCI
> nodes that won't have any children as long as no USB1-only
> peripheral is attached. ("usb tree" will only show EHCI nodes.)
>
> The failure can also be demonstrated with U-Boot's sandbox
> architecture, by simply enabling the inactive "usb_2" node in
> test.dts. This creates a similar situation, where the existing
> usb_device_info() implementation would crash (segfault) when
> issuing a "usb info" command.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
>  cmd/usb.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/usb.c b/cmd/usb.c
> index 58d9db2..3146f54 100644
> --- a/cmd/usb.c
> +++ b/cmd/usb.c
> @@ -602,18 +602,13 @@ static void show_info(struct udevice *dev)
>
>  static int usb_device_info(void)
>  {
> -	struct udevice *bus;
> -
> -	for (uclass_first_device(UCLASS_USB, &bus);
> -	     bus;
> -	     uclass_next_device(&bus)) {
> -		struct udevice *hub;
> +	struct udevice *hub;
>
> -		device_find_first_child(bus, &hub);
> -		if (device_get_uclass_id(hub) == UCLASS_USB_HUB &&
> -		    device_active(hub)) {
> +	for (uclass_first_device(UCLASS_USB_HUB, &hub);

We really want to check for bus here not hub, there are 2
problems with checking for hubs:

1) show_info recurses into child hubs, so going over all
hubs will cause any nested hubs to get printed twice
(I think)
2) on some otg  ports in host-mode there is no emulated
root-hub in u-boot, so this will skip printing of any device
attached to them

Besides this there are several other subtleties missed here
wrt iterating over usb-root-devs.

I've spend quite some time getting this all right for "usb tree"
but I somehow missed that "usb info" needed the same treatment.

I've prepared 2 patches which re-use the "usb tree" code
rather then re-inventing the wheel again:

https://github.com/jwrdegoede/u-boot-sunxi/commit/fecf9576247c9423a20570057a1de59f1dbc2da1
https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da800a354258a4ad2

As an added bonus these actually result in removing some code
instead of duplicating it.

Note these are only compile tested yet. I'll properly
test them tomorrow and then submit them officially.

Regards,

Hans

[U-Boot] [U-Boot, v1] dm: usb: Prevent NULL hub in usb_device_info()

[Hans de Goede]

Hi,

On 02-07-16 23:13, Hans de Goede wrote:
> Hi,
>
> On 29-06-16 10:43, Bernhard Nortmann wrote:
>> This patch modifies the usb_device_info() function to enumerate
>> active USB hubs by iterating UCLASS_USB_HUB directly.
>>
>> The previous code used UCLASS_USB nodes instead and retrieved
>> their first child node, expecting it to be a hub. However, it
>> did not protect against retrieving (and dereferencing) a NULL
>> pointer this way.
>>
>> Depending on the available USB hardware, this might happen easily.
>> For example the USB controller on sun7i-a20 has top-level OHCI
>> nodes that won't have any children as long as no USB1-only
>> peripheral is attached. ("usb tree" will only show EHCI nodes.)
>>
>> The failure can also be demonstrated with U-Boot's sandbox
>> architecture, by simply enabling the inactive "usb_2" node in
>> test.dts. This creates a similar situation, where the existing
>> usb_device_info() implementation would crash (segfault) when
>> issuing a "usb info" command.
>>
>> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  cmd/usb.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/cmd/usb.c b/cmd/usb.c
>> index 58d9db2..3146f54 100644
>> --- a/cmd/usb.c
>> +++ b/cmd/usb.c
>> @@ -602,18 +602,13 @@ static void show_info(struct udevice *dev)
>>
>>  static int usb_device_info(void)
>>  {
>> -    struct udevice *bus;
>> -
>> -    for (uclass_first_device(UCLASS_USB, &bus);
>> -         bus;
>> -         uclass_next_device(&bus)) {
>> -        struct udevice *hub;
>> +    struct udevice *hub;
>>
>> -        device_find_first_child(bus, &hub);
>> -        if (device_get_uclass_id(hub) == UCLASS_USB_HUB &&
>> -            device_active(hub)) {
>> +    for (uclass_first_device(UCLASS_USB_HUB, &hub);
>
> We really want to check for bus here not hub, there are 2
> problems with checking for hubs:
>
> 1) show_info recurses into child hubs, so going over all
> hubs will cause any nested hubs to get printed twice
> (I think)
> 2) on some otg  ports in host-mode there is no emulated
> root-hub in u-boot, so this will skip printing of any device
> attached to them
>
> Besides this there are several other subtleties missed here
> wrt iterating over usb-root-devs.
>
> I've spend quite some time getting this all right for "usb tree"
> but I somehow missed that "usb info" needed the same treatment.
>
> I've prepared 2 patches which re-use the "usb tree" code
> rather then re-inventing the wheel again:
>
> https://github.com/jwrdegoede/u-boot-sunxi/commit/fecf9576247c9423a20570057a1de59f1dbc2da1
> https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da800a354258a4ad2
>
> As an added bonus these actually result in removing some code
> instead of duplicating it.
>
> Note these are only compile tested yet. I'll properly
> test them tomorrow and then submit them officially.

Ok I've just completed a battery of tests and these 2 look good,
I'll submit them upstream right after this mail.

Regards,

Hans

[U-Boot] dm: usb: Prevent NULL hub in usb_device_info()

[Bernhard Nortmann]

Hi Hans!

Am 03.07.2016 um 20:21 schrieb Hans de Goede:
> Hi,
>
> On 02-07-16 23:13, Hans de Goede wrote:
>> Hi,
>> [...]
>>
>> I've prepared 2 patches which re-use the "usb tree" code
>> rather then re-inventing the wheel again:
>>
>> https://github.com/jwrdegoede/u-boot-sunxi/commit/fecf9576247c9423a20570057a1de59f1dbc2da1 
>>
>> https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da800a354258a4ad2 
>>
>>
>> As an added bonus these actually result in removing some code
>> instead of duplicating it.
>>
>> Note these are only compile tested yet. I'll properly
>> test them tomorrow and then submit them officially.
>
> Ok I've just completed a battery of tests and these 2 look good,
> I'll submit them upstream right after this mail.
>
> Regards,
>
> Hans


I can confirm that applying these two patches on top of 2016.07-rc3 
solves the
"usb info" problem I've been observing with sun7i-a20.

Tested-by: Bernhard Nortmann <bernhard.nortmann@web.de>

Regards, B. Nortmann

[U-Boot] dm: usb: Prevent NULL hub in usb_device_info()

[Marek Vasut]

On 07/03/2016 10:10 PM, Bernhard Nortmann wrote:
> Hi Hans!

Hi,

> Am 03.07.2016 um 20:21 schrieb Hans de Goede:
>> Hi,
>>
>> On 02-07-16 23:13, Hans de Goede wrote:
>>> Hi,
>>> [...]
>>>
>>> I've prepared 2 patches which re-use the "usb tree" code
>>> rather then re-inventing the wheel again:
>>>
>>> https://github.com/jwrdegoede/u-boot-sunxi/commit/fecf9576247c9423a20570057a1de59f1dbc2da1
>>>
>>> https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da800a354258a4ad2
>>>
>>>
>>> As an added bonus these actually result in removing some code
>>> instead of duplicating it.
>>>
>>> Note these are only compile tested yet. I'll properly
>>> test them tomorrow and then submit them officially.
>>
>> Ok I've just completed a battery of tests and these 2 look good,
>> I'll submit them upstream right after this mail.
>>
>> Regards,
>>
>> Hans
>
>
> I can confirm that applying these two patches on top of 2016.07-rc3
> solves the
> "usb info" problem I've been observing with sun7i-a20.
>
> Tested-by: Bernhard Nortmann <bernhard.nortmann@web.de>

OK, I'll pick them for 2016.07 .


-- 
Best regards,
Marek Vasut