[U-Boot] [PATCH] usb: check udev before dereferencing

[Simon Glass]

Hi,

On 12 November 2016 at 11:17, Marek Vasut <marex at denx.de> wrote:
> On 11/12/2016 07:10 PM, Anatolij Gustschin wrote:
>> On Sat, 12 Nov 2016 10:36:42 +0100
>> Marek Vasut marex at denx.de wrote:
>> ...
>>>>             udev = dev_get_parent_priv(child);
>>>> +           if (!udev)
>>>> +                   continue;
>>>
>>> I don't quite understand the problem from the patch description, but
>>> shouldn't all the return values from dev_get_parent_priv() be checked
>>> this way , not just these two ?
>>
>> The problem is that when dereferencing NULL udev we later access
>> some random address (e.g. when accessing dev->dev->parent in
>> usb_show_tree_graph()). dev->dev pointer is random DRAM data there,
>> when dereferencing it, data abort happens when random address
>> is outside of valid address range.
>
> I mean, I understand that udev can be NULL and we don't check it. But is
> udev == NULL an expected possibility ? And if so, when does such thing
> happen ?
>
>> Probably we should check elsewhere, at least where it might
>> return NULL.
>
> OK
>
>>>
>>> Why does dev_get_parent_priv() return NULL here ?
>>
>> it returns NULL because the dev->parent_priv is not allocated for
>> usb_mass_storage.lun0 device. I do not know the reason why.
>
> That's probably what needs to be fixed , no ?

Yes that seems wrong. There is a check immediately before this:

if (!device_active(child))
   continue;

If the device is active, it must have been probed and so must have parent data.

See:

UCLASS_DRIVER(usb) = {
...
.per_child_auto_alloc_size = sizeof(struct usb_device),

Try 'dm tree' to see what the bad USB device is.

>
> Also, we should most likely check all the return values of
> dev_get_parent_priv() in cmd/usb.c, not just these two.

Regards,
Simon