diff mbox

[U-Boot] usb: check udev before dereferencing

Message ID 1478941329-9539-1-git-send-email-agust@denx.de
State Rejected
Delegated to: Simon Glass
Headers show

Commit Message

Anatolij Gustschin Nov. 12, 2016, 9:02 a.m. UTC 
Fix crashes after data abort, e.g.:

    => usb start
    starting USB...
    USB0:   Core Release: 2.93a
    scanning bus 0 for devices... 3 USB Device(s) found

    => usb stor
    Device 0: Vendor: TOSHIBA  Rev: 1.00 Prod: TransMemory
    Type: Removable Hard Disk
    Capacity: 7400.0 MB = 7.2 GB (15155200 x 512)

    => usb tree
    USB device tree:
    1  Hub (480 Mb/s, 0mA)
    |   U-Boot Root Hub
    |
    +-2  Hub (480 Mb/s, 2mA)
    |
    +-3  Mass Storage (480 Mb/s, 200mA)
    |  TOSHIBA  TransMemory      DCE284AD740FCE4164F4095A
    |
    |data abort
    pc : [<3ff88990>]	   lr : [<3ff88a3f>]
    reloc pc : [<010149d0>]	   lr : [<01014a7f>]
    sp : 3bf6df70  ip : 00000000	 fp : 00000002
    r10: 3bf990b0  r9 : 3bf72ee8	 r8 : 3bf99090
    r7 : 3bf6e046  r6 : 00000006	 r5 : 3bf6e040  r4 : 00000000
    r3 : 80000000  r2 : 00000001	 r1 : 3bf6df74  r0 : effab9ca
    Flags: nzCv  IRQs off  FIQs off  Mode SVC_32
    Resetting CPU ...

Another example:

    => usb start
    starting USB...
    USB0:   Core Release: 2.93a
    scanning bus 0 for devices... 3 USB Device(s) found

    => usb stor
    Device 0: Vendor: TOSHIBA  Rev: 1.00 Prod: TransMemory
    Type: Removable Hard Disk
    Capacity: 7400.0 MB = 7.2 GB (15155200 x 512)

    => usb info
    1: Hub,  USB Revision 1.10
    -  U-Boot Root Hub
    - Class: Hub
    - PacketSize: 8  Configurations: 1
    - Vendor: 0x0000  Product 0x0000 Version 0.0
    Configuration: 1
    - Interfaces: 1 Self Powered 0mA
    Interface: 0
    - Alternate Setting 0, Endpoints: 1
    - Class Hub
    - Endpoint 1 In Interrupt MaxPacket 2 Interval 255ms

    2: Hub,  USB Revision 2.0
    - Class: Hub
    - PacketSize: 64  Configurations: 1
    - Vendor: 0x0424  Product 0x2512 Version 11.179
    Configuration: 1
    - Interfaces: 1 Self Powered Remote Wakeup 2mA
    Interface: 0
    - Alternate Setting 0, Endpoints: 1
    - Class Hub
    - Endpoint 1 In Interrupt MaxPacket 1 Interval 12ms
    - Endpoint 1 In Interrupt MaxPacket 1 Interval 12ms

    3: Mass Storage,  USB Revision 2.0
    - TOSHIBA  TransMemory      DCE284AD740FCE4164F4095A
    - Class: (from Interface) Mass Storage
    - PacketSize: 64  Configurations: 1
    - Vendor: 0x0930  Product 0x6544 Version 1.0
    Configuration: 1
    - Interfaces: 1 Bus Powered 200mA
    Interface: 0
    - Alternate Setting 0, Endpoints: 2
    - Class Mass Storage, Transp. SCSI, Bulk only
    - Endpoint 1 In Bulk MaxPacket 512
    - Endpoint 2 Out Bulk MaxPacket 512

    Configuration: 251
    - Interfaces: 249 Self Powered Remote Wakeup 502mA
    - data abort
    pc : [<3ff8e466>]	   lr : [<3ff8190f>]
    reloc pc : [<0101a4a6>]	   lr : [<0100d94f>]
    sp : 3bf6db48  ip : 00000000	 fp : 00000002
    r10: 000003bb  r9 : 3bf72ee8	 r8 : 00000064
    r7 : 00000000  r6 : 00000000	 r5 : 00000064  r4 : 3bf6db80
    r3 : 000000ff  r2 : 3bf6dc80	 r1 : dff5fee7  r0 : 7ad7efff
    Flags: NzCv  IRQs off  FIQs off  Mode SVC_32
    Resetting CPU ...

These craches are reproducible with CONFIG_BLK enabled.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 cmd/usb.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Marek Vasut Nov. 12, 2016, 9:36 a.m. UTC | #1 
On 11/12/2016 10:02 AM, Anatolij Gustschin wrote:
> Fix crashes after data abort, e.g.:
> 
>     => usb start
>     starting USB...
>     USB0:   Core Release: 2.93a
>     scanning bus 0 for devices... 3 USB Device(s) found
> 
>     => usb stor
>     Device 0: Vendor: TOSHIBA  Rev: 1.00 Prod: TransMemory
>     Type: Removable Hard Disk
>     Capacity: 7400.0 MB = 7.2 GB (15155200 x 512)
> 
>     => usb tree
>     USB device tree:
>     1  Hub (480 Mb/s, 0mA)
>     |   U-Boot Root Hub
>     |
>     +-2  Hub (480 Mb/s, 2mA)
>     |
>     +-3  Mass Storage (480 Mb/s, 200mA)
>     |  TOSHIBA  TransMemory      DCE284AD740FCE4164F4095A
>     |
>     |data abort
>     pc : [<3ff88990>]	   lr : [<3ff88a3f>]
>     reloc pc : [<010149d0>]	   lr : [<01014a7f>]
>     sp : 3bf6df70  ip : 00000000	 fp : 00000002
>     r10: 3bf990b0  r9 : 3bf72ee8	 r8 : 3bf99090
>     r7 : 3bf6e046  r6 : 00000006	 r5 : 3bf6e040  r4 : 00000000
>     r3 : 80000000  r2 : 00000001	 r1 : 3bf6df74  r0 : effab9ca
>     Flags: nzCv  IRQs off  FIQs off  Mode SVC_32
>     Resetting CPU ...
> 
> Another example:
> 
>     => usb start
>     starting USB...
>     USB0:   Core Release: 2.93a
>     scanning bus 0 for devices... 3 USB Device(s) found
> 
>     => usb stor
>     Device 0: Vendor: TOSHIBA  Rev: 1.00 Prod: TransMemory
>     Type: Removable Hard Disk
>     Capacity: 7400.0 MB = 7.2 GB (15155200 x 512)
> 
>     => usb info
>     1: Hub,  USB Revision 1.10
>     -  U-Boot Root Hub
>     - Class: Hub
>     - PacketSize: 8  Configurations: 1
>     - Vendor: 0x0000  Product 0x0000 Version 0.0
>     Configuration: 1
>     - Interfaces: 1 Self Powered 0mA
>     Interface: 0
>     - Alternate Setting 0, Endpoints: 1
>     - Class Hub
>     - Endpoint 1 In Interrupt MaxPacket 2 Interval 255ms
> 
>     2: Hub,  USB Revision 2.0
>     - Class: Hub
>     - PacketSize: 64  Configurations: 1
>     - Vendor: 0x0424  Product 0x2512 Version 11.179
>     Configuration: 1
>     - Interfaces: 1 Self Powered Remote Wakeup 2mA
>     Interface: 0
>     - Alternate Setting 0, Endpoints: 1
>     - Class Hub
>     - Endpoint 1 In Interrupt MaxPacket 1 Interval 12ms
>     - Endpoint 1 In Interrupt MaxPacket 1 Interval 12ms
> 
>     3: Mass Storage,  USB Revision 2.0
>     - TOSHIBA  TransMemory      DCE284AD740FCE4164F4095A
>     - Class: (from Interface) Mass Storage
>     - PacketSize: 64  Configurations: 1
>     - Vendor: 0x0930  Product 0x6544 Version 1.0
>     Configuration: 1
>     - Interfaces: 1 Bus Powered 200mA
>     Interface: 0
>     - Alternate Setting 0, Endpoints: 2
>     - Class Mass Storage, Transp. SCSI, Bulk only
>     - Endpoint 1 In Bulk MaxPacket 512
>     - Endpoint 2 Out Bulk MaxPacket 512
> 
>     Configuration: 251
>     - Interfaces: 249 Self Powered Remote Wakeup 502mA
>     - data abort
>     pc : [<3ff8e466>]	   lr : [<3ff8190f>]
>     reloc pc : [<0101a4a6>]	   lr : [<0100d94f>]
>     sp : 3bf6db48  ip : 00000000	 fp : 00000002
>     r10: 000003bb  r9 : 3bf72ee8	 r8 : 00000064
>     r7 : 00000000  r6 : 00000000	 r5 : 00000064  r4 : 3bf6db80
>     r3 : 000000ff  r2 : 3bf6dc80	 r1 : dff5fee7  r0 : 7ad7efff
>     Flags: NzCv  IRQs off  FIQs off  Mode SVC_32
>     Resetting CPU ...
> 
> These craches are reproducible with CONFIG_BLK enabled.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  cmd/usb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/cmd/usb.c b/cmd/usb.c
> index 455127c..80c8759 100644
> --- a/cmd/usb.c
> +++ b/cmd/usb.c
> @@ -410,6 +410,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
>  			continue;
>  
>  		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 ?

Why does dev_get_parent_priv() return NULL here ?

>  		/* Ignore emulators, we only want real devices */
>  		if (device_get_uclass_id(child) != UCLASS_USB_EMUL) {
> @@ -604,6 +606,8 @@ static void usb_show_info(struct usb_device *udev)
>  	     device_find_next_child(&child)) {
>  		if (device_active(child)) {
>  			udev = dev_get_parent_priv(child);
> +			if (!udev)
> +				continue;
>  			usb_show_info(udev);
>  		}
>  	}
>
Anatolij Gustschin Nov. 12, 2016, 6:10 p.m. UTC | #2 
On Sat, 12 Nov 2016 10:36:42 +0100
Marek Vasut marex@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.

Probably we should check elsewhere, at least where it might
return NULL.

> 
> 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.

--
Anatolij
Marek Vasut Nov. 12, 2016, 6:17 p.m. UTC | #3 
On 11/12/2016 07:10 PM, Anatolij Gustschin wrote:
> On Sat, 12 Nov 2016 10:36:42 +0100
> Marek Vasut marex@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 ?

Also, we should most likely check all the return values of
dev_get_parent_priv() in cmd/usb.c, not just these two.
Simon Glass Nov. 14, 2016, 8:44 p.m. UTC | #4 
Hi,

On 12 November 2016 at 11:17, Marek Vasut <marex@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@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
diff mbox

Patch

diff --git a/cmd/usb.c b/cmd/usb.c
index 455127c..80c8759 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -410,6 +410,8 @@  static void usb_show_tree_graph(struct usb_device *dev, char *pre)
 			continue;
 
 		udev = dev_get_parent_priv(child);
+		if (!udev)
+			continue;
 
 		/* Ignore emulators, we only want real devices */
 		if (device_get_uclass_id(child) != UCLASS_USB_EMUL) {
@@ -604,6 +606,8 @@  static void usb_show_info(struct usb_device *udev)
 	     device_find_next_child(&child)) {
 		if (device_active(child)) {
 			udev = dev_get_parent_priv(child);
+			if (!udev)
+				continue;
 			usb_show_info(udev);
 		}
 	}