[U-Boot] usb: hub enumeration and potential NULL ptr dereference in usb_device_info()
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
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:
- 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.
- 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
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:
- 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@01c14000 @ 7af37148, seq 0, (req -1) - * usb@01c14400 @ 7af371b0, seq 1, (req -1) - * usb@01c1c000 @ 7af37218, seq 2, (req -1) - * usb@01c1c400 @ 7af37280, seq 3, (req -1)
The root cause of the usb_device_info() problem here is that two of these controllers (ohci0: usb@01c14400, ohci1: usb@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.
- 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
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;
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
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/fecf9576247c9423a20570057a... https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da8...
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
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:
- 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/fecf9576247c9423a20570057a... https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da8...
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
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/fecf9576247c9423a20570057a...
https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da8...
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
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/fecf9576247c9423a20570057a...
https://github.com/jwrdegoede/u-boot-sunxi/commit/d61f186f494062539418ac9da8...
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 .
participants (4)
-
Bernhard Nortmann -
Hans de Goede -
Marek Vasut -
Simon Glass