usb: hub: fix panic caused by NULL bos pointer during reset device

From: "Du, Changbin" <[email protected]>

This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
hub: do not clear BOS field during reset device").

The privious one caused double mem-free if run to re_enumerate label. New patch title changed to distinguish from old one. And I have tested it with memory debugging options.

In function usb_reset_and_verify_device, the old BOS descriptor may still be used before allocating a new one. (usb_disable_lpm function uses it under the situation that it fails at usb_disable_link_state.) So we cannot set the udev->bos to NULL before that, just keep what it was. It will be overwrite when allocating a new one.

How to reproduce:
1. connect one usb3 hub to xhci port.
2. connect several lpm-capable super-speed usb disk to the hub.
3. copy big files to the usb disks.
4. disconnect the hub and repeat step 1-4.

Crash log:

BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
IP: [<ffffffff8171f98d>] usb_enable_link_state+0x2d/0x2f0
Call Trace:
[<ffffffff8171ed5b>] ? usb_set_lpm_timeout+0x12b/0x140
[<ffffffff8171fcd1>] usb_enable_lpm+0x81/0xa0
[<ffffffff8171fdd8>] usb_disable_lpm+0xa8/0xc0
[<ffffffff8171fe1c>] usb_unlocked_disable_lpm+0x2c/0x50
[<ffffffff81723933>] usb_reset_and_verify_device+0xc3/0x710
[<ffffffff8172c4ed>] ? usb_sg_wait+0x13d/0x190
[<ffffffff81724743>] usb_reset_device+0x133/0x280
[<ffffffff8179ccd1>] usb_stor_port_reset+0x61/0x70
[<ffffffff8179cd68>] usb_stor_invoke_transport+0x88/0x520

Signed-off-by Du, Changbin <[email protected]>

---
 drivers/usb/core/hub.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

drivers/usb/core/hub.c +9 -5

index 51b43691..27444e5 100644
@@ -5401,7 +5401,6 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
 	}
 
 	bos = udev->bos;
-	udev->bos = NULL;
 
 	for (i = 0; i < SET_CONFIG_TRIES; ++i) {
 

@@ -5494,13 +5493,18 @@ done:
 	usb_set_usb2_hardware_lpm(udev, 1);
 	usb_unlocked_enable_lpm(udev);
 	usb_enable_ltm(udev);
-	usb_release_bos_descriptor(udev);
-	udev->bos = bos;
+	/* release the new BOS descriptor allocated by hub_port_init() */
+	if (udev->bos != bos) {
+		usb_release_bos_descriptor(udev);
+		udev->bos = bos;
+	}
 	return 0;
 
 re_enumerate:
-	usb_release_bos_descriptor(udev);
-	udev->bos = bos;
+	if (udev->bos != bos) {
+		usb_release_bos_descriptor(udev);
+		udev->bos = bos;
+	}
 re_enumerate_no_bos:
 	/* LPM state doesn't matter when we're about to destroy the device. */
 	hub_port_logical_disconnect(parent_hub, port1);

Signature · 6 lines

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 36 lines

From: "Du, Changbin" <[email protected]>

This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
hub: do not clear BOS field during reset device").

The privious one caused double mem-free if run to re_enumerate label. New patch title changed to distinguish from old one. And I have tested it with memory debugging options.

In function usb_reset_and_verify_device, the old BOS descriptor may still be used before allocating a new one. (usb_disable_lpm function uses it under the situation that it fails at usb_disable_link_state.) So we cannot set the udev->bos to NULL before that, just keep what it was. It will be overwrite when allocating a new one.

How to reproduce:
1. connect one usb3 hub to xhci port.
2. connect several lpm-capable super-speed usb disk to the hub.
3. copy big files to the usb disks.
4. disconnect the hub and repeat step 1-4.

Crash log:

BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
IP: [<ffffffff8171f98d>] usb_enable_link_state+0x2d/0x2f0
Call Trace:
[<ffffffff8171ed5b>] ? usb_set_lpm_timeout+0x12b/0x140
[<ffffffff8171fcd1>] usb_enable_lpm+0x81/0xa0
[<ffffffff8171fdd8>] usb_disable_lpm+0xa8/0xc0
[<ffffffff8171fe1c>] usb_unlocked_disable_lpm+0x2c/0x50
[<ffffffff81723933>] usb_reset_and_verify_device+0xc3/0x710
[<ffffffff8172c4ed>] ? usb_sg_wait+0x13d/0x190
[<ffffffff81724743>] usb_reset_device+0x133/0x280
[<ffffffff8179ccd1>] usb_stor_port_reset+0x61/0x70
[<ffffffff8179cd68>] usb_stor_invoke_transport+0x88/0x520

Quoted 4 lines

Signed-off-by Du, Changbin <[email protected]>

---
 drivers/usb/core/hub.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Is this patch still needed? I thought we had some other fix in this area...

confused,

greg k-h

Quoted 41 lines · On Tue, Mar 08, 2016 at 05:15:17PM +0800, [email protected] wrote:

Quoted 36 lines

From: "Du, Changbin" <[email protected]>

This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
hub: do not clear BOS field during reset device").

The privious one caused double mem-free if run to re_enumerate label. New patch title changed to distinguish from old one. And I have tested it with memory debugging options.

In function usb_reset_and_verify_device, the old BOS descriptor may still be used before allocating a new one. (usb_disable_lpm function uses it under the situation that it fails at usb_disable_link_state.) So we cannot set the udev->bos to NULL before that, just keep what it was. It will be overwrite when allocating a new one.

How to reproduce:
1. connect one usb3 hub to xhci port.
2. connect several lpm-capable super-speed usb disk to the hub.
3. copy big files to the usb disks.
4. disconnect the hub and repeat step 1-4.

Crash log:

BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
IP: [<ffffffff8171f98d>] usb_enable_link_state+0x2d/0x2f0
Call Trace:
[<ffffffff8171ed5b>] ? usb_set_lpm_timeout+0x12b/0x140
[<ffffffff8171fcd1>] usb_enable_lpm+0x81/0xa0
[<ffffffff8171fdd8>] usb_disable_lpm+0xa8/0xc0
[<ffffffff8171fe1c>] usb_unlocked_disable_lpm+0x2c/0x50
[<ffffffff81723933>] usb_reset_and_verify_device+0xc3/0x710
[<ffffffff8172c4ed>] ? usb_sg_wait+0x13d/0x190
[<ffffffff81724743>] usb_reset_device+0x133/0x280
[<ffffffff8179ccd1>] usb_stor_port_reset+0x61/0x70
[<ffffffff8179cd68>] usb_stor_invoke_transport+0x88/0x520

Quoted 4 lines

Signed-off-by Du, Changbin <[email protected]>

---
 drivers/usb/core/hub.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Quoted 7 lines

Is this patch still needed? I thought we had some other fix in this area...

confused,

greg k-h

Hi, Greg k-h,
Sorry for it confused you. This patch still need. This is same fix with previous commit d8f00cd685f5 ("usb: hub: do not clear BOS field during reset device"). But d8f00cd685f5 is buggy and reverted. This new patch should be the final fix.

Best Regards,
Du, Changbin

Quoted 48 lines · Changbin

Quoted 41 lines · On Tue, Mar 08, 2016 at 05:15:17PM +0800, [email protected] wrote:

Quoted 36 lines

From: "Du, Changbin" <[email protected]>

This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
hub: do not clear BOS field during reset device").

The privious one caused double mem-free if run to re_enumerate label. New patch title changed to distinguish from old one. And I have tested it with memory debugging options.

In function usb_reset_and_verify_device, the old BOS descriptor may still be used before allocating a new one. (usb_disable_lpm function uses it under the situation that it fails at usb_disable_link_state.) So we cannot set the udev->bos to NULL before that, just keep what it was. It will be overwrite when allocating a new one.

How to reproduce:
1. connect one usb3 hub to xhci port.
2. connect several lpm-capable super-speed usb disk to the hub.
3. copy big files to the usb disks.
4. disconnect the hub and repeat step 1-4.

Crash log:

BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
IP: [<ffffffff8171f98d>] usb_enable_link_state+0x2d/0x2f0
Call Trace:
[<ffffffff8171ed5b>] ? usb_set_lpm_timeout+0x12b/0x140
[<ffffffff8171fcd1>] usb_enable_lpm+0x81/0xa0
[<ffffffff8171fdd8>] usb_disable_lpm+0xa8/0xc0
[<ffffffff8171fe1c>] usb_unlocked_disable_lpm+0x2c/0x50
[<ffffffff81723933>] usb_reset_and_verify_device+0xc3/0x710
[<ffffffff8172c4ed>] ? usb_sg_wait+0x13d/0x190
[<ffffffff81724743>] usb_reset_device+0x133/0x280
[<ffffffff8179ccd1>] usb_stor_port_reset+0x61/0x70
[<ffffffff8179cd68>] usb_stor_invoke_transport+0x88/0x520

Quoted 4 lines

Signed-off-by Du, Changbin <[email protected]>

---
 drivers/usb/core/hub.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Quoted 7 lines

Is this patch still needed? I thought we had some other fix in this area...

confused,

greg k-h

Quoted 9 lines

Hi, Greg k-h,
Sorry for it confused you. This patch still need. This is same fix with previous commit d8f00cd685f5 ("usb: hub: do not clear BOS field during reset device"). But d8f00cd685f5 is buggy and reverted. This new patch should be the final fix.

Best Regards,
Du, Changbin

I think Greg is referring to commit 464ad8c43a9e ("usb: core : hub: Fix BOS 'NULL pointer' kernel panic"), which has already been applied upstream. It looks to me like that patch might have fixed the same problem in a different way, in which case Changbin's patch is not needed. But I haven't been involved in developing or testing that patch, so I can't say for sure. At the very least, 464ad8c43a9e conflicts with Changbin's patch.

Changbin, can you take a look at 464ad8c43a9e and see if that fixes the same problem that your patch did?

Quoted 10 lines

I think Greg is referring to commit 464ad8c43a9e ("usb: core : hub: Fix BOS 'NULL pointer' kernel panic"), which has already been applied upstream. It looks to me like that patch might have fixed the same problem in a different way, in which case Changbin's patch is not needed. But I haven't been involved in developing or testing that patch, so I can't say for sure. At the very least, 464ad8c43a9e conflicts with Changbin's patch.

Changbin, can you take a look at 464ad8c43a9e and see if that fixes the same problem that your patch did?

Given the lack of response here, I've dropped this from my queue. If it's still needed, someone must resend it.

thanks,

greg k-h