Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1384960 > unrolled thread

[PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Jim Lin <jilin@nvidia.com> - 2016-04-22 12:50 +0200
    Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Lars-Peter Clausen <lars@metafoo.de> - 2016-04-22 13:30 +0200
    Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Felipe Balbi <balbi@kernel.org> - 2016-04-22 14:00 +0200
      Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Jim Lin <jilin@nvidia.com> - 2016-04-25 13:40 +0200
        Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Felipe Balbi <balbi@kernel.org> - 2016-04-25 14:10 +0200
          Re: [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed Jim Lin <jilin@nvidia.com> - 2016-04-26 11:00 +0200

#1384960 — [PATCH] usb: gadget: f_fs: Fix kernel panic for SuperSpeed

Android N adds os_desc_compat in v2_descriptor by init_functionfs()
(system/core/adb/usb_linux_client.cpp) to support automatic install
of MTP driver on Windows for USB device mode.

Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
and return -EINVAL.
This results in a second adb_write of usb_linux_client.cpp
(system/core/adb/) which doesn't have ss_descriptors filled.
Then later kernel_panic (composite.c) occurs when ss_descriptors
as a pointer with NULL is being accessed.

Fix is to ignore the checking on reserved1 field so that first
adb_write goes successfully with v2_descriptor which has
ss_descriptors filled.

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d5..f5ea3df 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2050,8 +2050,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
 		int i;
 
 		if (len < sizeof(*d) ||
-		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-		    d->Reserved1)
+		    d->bFirstInterfaceNumber >= ffs->interfaces_count)
 			return -EINVAL;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])
-- 
1.9.1

[toc] | [next] | [standalone]

#1385055

On 04/22/2016 12:43 PM, Jim Lin wrote:
> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
> (system/core/adb/usb_linux_client.cpp) to support automatic install
> of MTP driver on Windows for USB device mode.
> 
> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
> and return -EINVAL.
> This results in a second adb_write of usb_linux_client.cpp
> (system/core/adb/) which doesn't have ss_descriptors filled.
> Then later kernel_panic (composite.c) occurs when ss_descriptors
> as a pointer with NULL is being accessed.
> 
> Fix is to ignore the checking on reserved1 field so that first
> adb_write goes successfully with v2_descriptor which has
> ss_descriptors filled.

That sounds like the wrong approach. The kernel should not crash if
ss_descriptors is not filled. I think the right fix is to make sure that the
NULL pointer deref can never happen regardless of which input is supplied by
userspace.

[toc] | [prev] | [next] | [standalone]

#1385107

[Multipart message — attachments visible in raw view] — view raw

Hi Jim,

Jim Lin <jilin@nvidia.com> writes:
> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
> (system/core/adb/usb_linux_client.cpp) to support automatic install
> of MTP driver on Windows for USB device mode.
>
> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
> and return -EINVAL.
> This results in a second adb_write of usb_linux_client.cpp
> (system/core/adb/) which doesn't have ss_descriptors filled.
> Then later kernel_panic (composite.c) occurs when ss_descriptors
> as a pointer with NULL is being accessed.

where exactly in composite.c are we dereferencing a NULL pointer ?

Is this happening on set_config() ? If that's the case, why is
gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
should already have negotiated highspeed which means
function_descriptors() should have returned highspeed descriptors, not a
NULL superspeed.

Care to explain why you haven't negotiated Highspeed ? The only thing I
can think of is that you're using a Superspeed-capable peripheral
controller (dwc3?) with maximum-speed set to Superspeed, with a
Superspeed-capable cable connected to an XHCI PC, but loading a
high-speed gadget driver (which you got from Android, written with f_fs)
and this gadget doesn't tell composite that its maximum speed is
Highspeed, instead of super-speed.

We can add a check, sure, to avoid a kernel oops; however, you should
really fix up the gadget implementation and/or set dwc3's maximum-speed
property accordingly.

Can you check if this patch makes your scenario work while still being
fully functional ?

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index de9ffd60fcfa..3d3cdc5ed20d 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
 {
 	struct usb_descriptor_header **descriptors;
 
+	/*
+	 * NOTE: we try to help gadget drivers which might not be setting
+	 * max_speed appropriately.
+	 */
+
 	switch (speed) {
 	case USB_SPEED_SUPER_PLUS:
 		descriptors = f->ssp_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	case USB_SPEED_SUPER:
 		descriptors = f->ss_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	case USB_SPEED_HIGH:
 		descriptors = f->hs_descriptors;
-		break;
+		if (descriptors)
+			break;
+		/* FALLTHROUGH */
 	default:
 		descriptors = f->fs_descriptors;
 	}
 
+	/*
+	 * if we can't find any descriptors at all, then this gadget deserves to
+	 * Oops with a NULL pointer dereference
+	 */
+
 	return descriptors;
 }
 
-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1386280

On 2016年04月22日 19:52, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
> Hi Jim,
>
> Jim Lin <jilin@nvidia.com> writes:
>> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
>> (system/core/adb/usb_linux_client.cpp) to support automatic install
>> of MTP driver on Windows for USB device mode.
>>
>> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
>> and return -EINVAL.
>> This results in a second adb_write of usb_linux_client.cpp
>> (system/core/adb/) which doesn't have ss_descriptors filled.
>> Then later kernel_panic (composite.c) occurs when ss_descriptors
>> as a pointer with NULL is being accessed.
> where exactly in composite.c are we dereferencing a NULL pointer ?
In set_config

for (; *descriptors; ++descriptors) {



Here is the link which shows reserved (at offset 1, e.g., Function 
Section 2) as  1.
https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx


>
> Is this happening on set_config() ? If that's the case, why is
> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
> should already have negotiated highspeed which means
> function_descriptors() should have returned highspeed descriptors, not a
> NULL superspeed.
>
> Care to explain why you haven't negotiated Highspeed ? The only thing I
> can think of is that you're using a Superspeed-capable peripheral
> controller (dwc3?) with maximum-speed set to Superspeed, with a
> Superspeed-capable cable connected to an XHCI PC, but loading a
> high-speed gadget driver (which you got from Android, written with f_fs)
> and this gadget doesn't tell composite that its maximum speed is
> Highspeed, instead of super-speed.
>
> We can add a check, sure, to avoid a kernel oops; however, you should
> really fix up the gadget implementation and/or set dwc3's maximum-speed
> property accordingly.
>
> Can you check if this patch makes your scenario work while still being
> fully functional ?
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index de9ffd60fcfa..3d3cdc5ed20d 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>   {
>   	struct usb_descriptor_header **descriptors;
>   
> +	/*
> +	 * NOTE: we try to help gadget drivers which might not be setting
> +	 * max_speed appropriately.
> +	 */
> +
>   	switch (speed) {
>   	case USB_SPEED_SUPER_PLUS:
>   		descriptors = f->ssp_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	case USB_SPEED_SUPER:
>   		descriptors = f->ss_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	case USB_SPEED_HIGH:
>   		descriptors = f->hs_descriptors;
> -		break;
> +		if (descriptors)
> +			break;
> +		/* FALLTHROUGH */
>   	default:
>   		descriptors = f->fs_descriptors;
>   	}
>   
> +	/*
> +	 * if we can't find any descriptors at all, then this gadget deserves to
> +	 * Oops with a NULL pointer dereference
> +	 */
> +
>   	return descriptors;
>   }
>   
After trying your change, no kernel panic, but SuperSpeed device (device 
mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP 
device with USB3.0 cable.

--nvpublic

[toc] | [prev] | [next] | [standalone]

#1386323

[Multipart message — attachments visible in raw view] — view raw

Hi,

Jim Lin <jilin@nvidia.com> writes:
> On 2016年04月22日 19:52, Felipe Balbi wrote:
>> * PGP Signed by an unknown key
>>
>>
>> Hi Jim,
>>
>> Jim Lin <jilin@nvidia.com> writes:
>>> Android N adds os_desc_compat in v2_descriptor by init_functionfs()
>>> (system/core/adb/usb_linux_client.cpp) to support automatic install
>>> of MTP driver on Windows for USB device mode.
>>>
>>> Current __ffs_data_do_os_desc() of f_fs.c will check reserved1 field
>>> and return -EINVAL.
>>> This results in a second adb_write of usb_linux_client.cpp
>>> (system/core/adb/) which doesn't have ss_descriptors filled.
>>> Then later kernel_panic (composite.c) occurs when ss_descriptors
>>> as a pointer with NULL is being accessed.
>> where exactly in composite.c are we dereferencing a NULL pointer ?
> In set_config
>
> for (; *descriptors; ++descriptors) {
>
>
>
> Here is the link which shows reserved (at offset 1, e.g., Function 
> Section 2) as  1.
> https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx

thanks

>> Is this happening on set_config() ? If that's the case, why is
>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>> should already have negotiated highspeed which means
>> function_descriptors() should have returned highspeed descriptors, not a
>> NULL superspeed.
>>
>> Care to explain why you haven't negotiated Highspeed ? The only thing I
>> can think of is that you're using a Superspeed-capable peripheral
>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>> Superspeed-capable cable connected to an XHCI PC, but loading a
>> high-speed gadget driver (which you got from Android, written with f_fs)
>> and this gadget doesn't tell composite that its maximum speed is
>> Highspeed, instead of super-speed.
>>
>> We can add a check, sure, to avoid a kernel oops; however, you should
>> really fix up the gadget implementation and/or set dwc3's maximum-speed
>> property accordingly.
>>
>> Can you check if this patch makes your scenario work while still being
>> fully functional ?
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>   {
>>   	struct usb_descriptor_header **descriptors;
>>   
>> +	/*
>> +	 * NOTE: we try to help gadget drivers which might not be setting
>> +	 * max_speed appropriately.
>> +	 */
>> +
>>   	switch (speed) {
>>   	case USB_SPEED_SUPER_PLUS:
>>   		descriptors = f->ssp_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	case USB_SPEED_SUPER:
>>   		descriptors = f->ss_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	case USB_SPEED_HIGH:
>>   		descriptors = f->hs_descriptors;
>> -		break;
>> +		if (descriptors)
>> +			break;
>> +		/* FALLTHROUGH */
>>   	default:
>>   		descriptors = f->fs_descriptors;
>>   	}
>>   
>> +	/*
>> +	 * if we can't find any descriptors at all, then this gadget deserves to
>> +	 * Oops with a NULL pointer dereference
>> +	 */
>> +
>>   	return descriptors;
>>   }
>>   
> After trying your change, no kernel panic, but SuperSpeed device (device 
> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP 
> device with USB3.0 cable.

what do you get on dmesg on host side ? Are you running dwc3 ? If you
are, please capture trace logs of the failure:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 2048 > buffer_size_kb
# echo 1 > events/dwc3/enable

(now connect your cable to host pc)

# cp trace /path/to/non-volatile/storage/trace.txt

Please reply with this trace.txt file and dmesg from host side.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1387235

On 2016年04月25日 20:01, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
>
>
>
>>> Is this happening on set_config() ? If that's the case, why is
>>> gadget->speed set to USB_SPEED_SUPER to start with ? Your controller
>>> should already have negotiated highspeed which means
>>> function_descriptors() should have returned highspeed descriptors, not a
>>> NULL superspeed.
>>>
>>> Care to explain why you haven't negotiated Highspeed ? The only thing I
>>> can think of is that you're using a Superspeed-capable peripheral
>>> controller (dwc3?) with maximum-speed set to Superspeed, with a
>>> Superspeed-capable cable connected to an XHCI PC, but loading a
>>> high-speed gadget driver (which you got from Android, written with f_fs)
>>> and this gadget doesn't tell composite that its maximum speed is
>>> Highspeed, instead of super-speed.
>>>
>>> We can add a check, sure, to avoid a kernel oops; however, you should
>>> really fix up the gadget implementation and/or set dwc3's maximum-speed
>>> property accordingly.
>>>
>>> Can you check if this patch makes your scenario work while still being
>>> fully functional ?
>>>
>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>> index de9ffd60fcfa..3d3cdc5ed20d 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -66,20 +66,36 @@ function_descriptors(struct usb_function *f,
>>>    {
>>>    	struct usb_descriptor_header **descriptors;
>>>    
>>> +	/*
>>> +	 * NOTE: we try to help gadget drivers which might not be setting
>>> +	 * max_speed appropriately.
>>> +	 */
>>> +
>>>    	switch (speed) {
>>>    	case USB_SPEED_SUPER_PLUS:
>>>    		descriptors = f->ssp_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	case USB_SPEED_SUPER:
>>>    		descriptors = f->ss_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	case USB_SPEED_HIGH:
>>>    		descriptors = f->hs_descriptors;
>>> -		break;
>>> +		if (descriptors)
>>> +			break;
>>> +		/* FALLTHROUGH */
>>>    	default:
>>>    		descriptors = f->fs_descriptors;
>>>    	}
>>>    
>>> +	/*
>>> +	 * if we can't find any descriptors at all, then this gadget deserves to
>>> +	 * Oops with a NULL pointer dereference
>>> +	 */
>>> +
>>>    	return descriptors;
>>>    }
>>>    
>> After trying your change, no kernel panic, but SuperSpeed device (device
>> mode) is not enumerated by ubuntu 14.04 USB 3.0 host controller as MTP
>> device with USB3.0 cable.
> what do you get on dmesg on host side ? Are you running dwc3 ? If you
> are, please capture trace logs of the failure:
>
> # mount -t debugfs none /sys/kernel/debug
> # cd /sys/kernel/debug/tracing
> # echo 2048 > buffer_size_kb
> # echo 1 > events/dwc3/enable
>
> (now connect your cable to host pc)
>
> # cp trace /path/to/non-volatile/storage/trace.txt
>
> Please reply with this trace.txt file and dmesg from host side.
This is not running with dwc3.

dmesg from PC host side (after adding your change without my patch):

[17907.984647] usb 6-2: new SuperSpeed USB device number 54 using xhci_hcd
[17908.012036] usb 6-2: No SuperSpeed endpoint companion for config 1  
interface 1 altsetting 0 ep 2: using minimum values
[17908.012040] usb 6-2: No SuperSpeed endpoint companion for config 1  
interface 1 altsetting 0 ep 131: using minimum values
[17908.013652] usb 6-2: New USB device found, idVendor=xxxx, idProduct=xxxx
[17908.013656] usb 6-2: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3
[17908.013658] usb 6-2: Product: xxxxxxx
[17908.013661] usb 6-2: Manufacturer: xxxxxx
[17908.013664] usb 6-2: SerialNumber: 1234567890
[17908.014680] xhci_hcd 0000:05:00.0: ERROR: unexpected command 
completion code 0x11.
[17908.014690] usb 6-2: can't set config #1, error -22

I also attach git log of system/core/adb/usb_linux_client.cpp of Android 
N for your reference.
"
Author: Badhri Jagan Sridharan <Badhri@google.com>
Date:   Mon Oct 5 13:04:03 2015 -0700

     adbd: Add os descriptor support for adb.

     Eventhough windows does not rely on extended os
     descriptor for adbd, when android usb device is
     configures as a composite device such as mtp+adb,
     windows discards the extended os descriptor even
     if one of the USB function fails to send
     the extended compat descriptor. This results in automatic
     install of MTP driverto fail when Android device is in
     "File Transfer" mode with adb enabled.

https://msdn.microsoft.com/en-us/library/windows/hardware/gg463179.aspx
"

--nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web