usb: check udev before dereferencing

PATCH New Anatolij Gustschin 3 participants 5 messages Nov 12, 2016 – Nov 14, 2016

lore

Share & tools

lore

Anatolij Gustschin

author

2016-11-12 09:02 UTC #0 lore raw

PATCH 1/1 usb: check udev before dereferencing

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 <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Simon Glass <[email protected]>

---
 cmd/usb.c | 4 ++++
 1 file changed, 4 insertions(+)

cmd/usb.c +4 -0

index 455127c..80c8759 100644
@@ -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);
 		}
 	}

Signature · 1 line

2.7.4

Marek Vasut

2016-11-12 09:36 UTC #1 lore raw

PATCH 1/1 usb: check udev before dereferencing

Quoted 108 lines · Anatolij Gustschin

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 <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
---
 cmd/usb.c | 4 ++++
 1 file changed, 4 insertions(+)
cmd/usb.c +0 -0
index 455127c..80c8759 100644

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 414

Quoted 5 lines

cmd/usb.c +2 -0

@@ -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 ?

Quoted 12 lines

cmd/usb.c +2 -0

 		/* 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);
 		}
 	}

Signature · 2 lines

Best regards,
Marek Vasut

Anatolij Gustschin

author

2016-11-12 18:10 UTC #2 lore raw

PATCH 1/1 usb: check udev before dereferencing

On Sat, 12 Nov 2016 10:36:42 +0100
Marek Vasut marex at denx.de wrote:
...

Quoted 7 lines

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 414
Quoted 3 lines
cmd/usb.c +2 -0
 		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.

Quoted 2 lines

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

2016-11-12 18:17 UTC #3 lore raw

PATCH 1/1 usb: check udev before dereferencing

Quoted 11 lines · Anatolij Gustschin

On Sat, 12 Nov 2016 10:36:42 +0100
Marek Vasut marex at denx.de wrote:
...
Quoted 7 lines
cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 414
Quoted 3 lines
cmd/usb.c +2 -0
 		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 ?

Quoted 5 lines

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 ?

Quoted 2 lines

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

Quoted 5 lines

Quoted 2 lines
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.

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2016-11-14 20:44 UTC #4 lore raw

PATCH 1/1 usb: check udev before dereferencing

Hi,

Quoted 28 lines · Marek Vasut

Quoted 11 lines · Anatolij Gustschin
On Sat, 12 Nov 2016 10:36:42 +0100
Marek Vasut marex at denx.de wrote:
...
Quoted 7 lines
Review
Quoted 3 lines
            udev = dev_get_parent_priv(child);
(quoted diff) +2 -0
+           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 ?
Quoted 5 lines
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 ?
Quoted 2 lines
Probably we should check elsewhere, at least where it might return NULL.
OK
Quoted 1 line
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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.

Quoted 3 lines

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