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

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

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

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 <changbin.du@intel.com>
---
 drivers/usb/core/hub.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 51b43691..27444e5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -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);
-- 
2.5.0

Re: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

[Greg KH]

On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
> 
> 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 <changbin.du@intel.com>
> ---
>  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


> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 51b43691..27444e5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -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);
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

[Du, Changbin]

> On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > 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 <changbin.du@intel.com>
> > ---
> >  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
> 

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

Re: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

[Tony Battersby]

On 04/26/2016 10:53 PM, Du, Changbin wrote:
>> On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin.du@intel.com wrote:
>>> From: "Du, Changbin" <changbin.du@intel.com>
>>>
>>> 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 <changbin.du@intel.com>
>>> ---
>>>  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
>>
> 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?

Thanks,
Tony Battersby

Re: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

[Greg KH]

On Wed, Apr 27, 2016 at 09:35:57AM -0400, Tony Battersby wrote:
> On 04/26/2016 10:53 PM, Du, Changbin wrote:
> >> On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin.du@intel.com wrote:
> >>> From: "Du, Changbin" <changbin.du@intel.com>
> >>>
> >>> 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 <changbin.du@intel.com>
> >>> ---
> >>>  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
> >>
> > 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?

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

RE: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

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

Hi,
I missed this email because it was junked by my email client. Just checked
patch 464ad8c43a9e, it fix the same issue. So my patch no longer need now.
Thanks for the patch.

Best Regards,
Du, Changbin