Re: [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  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@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, ) 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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  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, );
> +printf("bus %p, uclass %d, hub %p: ",
> +bus, device_get_uclass_id(bus), hub);
> +if (!hub) {
> +printf("\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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot