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

[Marek Vasut]

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 at denx.de>
> Cc: Marek Vasut <marex at denx.de>
> Cc: Simon Glass <sjg at 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);
>  		}
>  	}
> 


-- 
Best regards,
Marek Vasut