Patchwork Guest OS hangs on usb_add

login
register
mail settings
Submitter TJ
Date June 24, 2010, 4:45 a.m.
Message ID <4C22E300.5020109@gmail.com>
Download mbox | patch
Permalink /patch/56758/
State New
Headers show

Comments

TJ - June 24, 2010, 4:45 a.m.
> ---------- Forwarded message ----------
> From: Timothy Jones <one.timothy.jones@gmail.com>
> Date: Wed, Jun 23, 2010 at 9:07 PM
> Subject: Guest OS hangs on usb_add
> To: qemu-devel@nongnu.org
> 
> 
> With some digging around I found out that the qemu hangs in
> usb_host_claim_interfaces, which is caused by screwed up usb
> descriptor. The device reports the following:
> 
> (gdb) p dev->descr_len
> $21 = 50
> (gdb) p /x dev->descr[0]@50
> $23 = {0x18, 0x1, 0x0, 0x1, 0xff, 0xff, 0xff, 0x8, 0x47, 0x46, 0x0,
> 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x9, 0x2, 0x20,
>  0x0, 0x1, 0x1, 0x0, 0x80, 0x19, 0x9, 0x4, 0x0, 0x0, 0x2, 0xff, 0xff,
> 0xff, 0x0, 0x7, 0x5, 0x81, 0x2, 0x40, 0x0, 0x0,
>  0x7, 0x5, 0x3, 0x2, 0x10, 0x0, 0x0}
> 
> The first 0x18 (Device Descriptor bLength) is supposed to be decimal
> 18, not hex! According to USB spec, if the device reports size greater
> than expected, the host is supposed ignore the extra bytes. So qemu
> behaves correctly here. However, with this length, the following
> Configuration Descriptor length falls on a 0x0 and so the qemu spins
> in an endless loop. (This is prolly something that should be detected
> and reported as error by qemu.)
> 
> My question is: This 0x18 -- is this something that comes from the
> device itself (ie, firmware bug)? Or does it come from the USB
> subsystem?
> 
> I don't mind writing a small patch to make descriptor parsing a bit
> more intelligent, but I am very unfamiliar with the code, so I might
> botch things up. Or is the above data sufficient for one of the devs
> to take a look at the code and improve it?
> 
> Thank you.
> 
> -TJ
> 

Here is small patch that fixed my problem.

In looking at the USB spec, it seems pretty clear cut about the whole
device/config/interface/endpoint descriptor hierarchy, so the
usb_host_claim_interfaces can be optimized instead of parsing through each
descriptor to skip through config descriptors using wTotalLength field. And
again, some checks can be done for descriptor types and/or sizes.

Just my 2 cents.

-TJ
David S. Ahern - June 24, 2010, 5:59 p.m.
On 06/23/10 22:45, TJ wrote:
> 
>> ---------- Forwarded message ----------
>> From: Timothy Jones <one.timothy.jones@gmail.com>
>> Date: Wed, Jun 23, 2010 at 9:07 PM
>> Subject: Guest OS hangs on usb_add
>> To: qemu-devel@nongnu.org
>>
>>
>> With some digging around I found out that the qemu hangs in
>> usb_host_claim_interfaces, which is caused by screwed up usb
>> descriptor. The device reports the following:
>>
>> (gdb) p dev->descr_len
>> $21 = 50
>> (gdb) p /x dev->descr[0]@50
>> $23 = {0x18, 0x1, 0x0, 0x1, 0xff, 0xff, 0xff, 0x8, 0x47, 0x46, 0x0,
>> 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x9, 0x2, 0x20,
>>  0x0, 0x1, 0x1, 0x0, 0x80, 0x19, 0x9, 0x4, 0x0, 0x0, 0x2, 0xff, 0xff,
>> 0xff, 0x0, 0x7, 0x5, 0x81, 0x2, 0x40, 0x0, 0x0,
>>  0x7, 0x5, 0x3, 0x2, 0x10, 0x0, 0x0}
>>
>> The first 0x18 (Device Descriptor bLength) is supposed to be decimal
>> 18, not hex! According to USB spec, if the device reports size greater
>> than expected, the host is supposed ignore the extra bytes. So qemu
>> behaves correctly here. However, with this length, the following
>> Configuration Descriptor length falls on a 0x0 and so the qemu spins
>> in an endless loop. (This is prolly something that should be detected
>> and reported as error by qemu.)
>>
>> My question is: This 0x18 -- is this something that comes from the
>> device itself (ie, firmware bug)? Or does it come from the USB
>> subsystem?

What kind of device is this?

David
TJ - June 24, 2010, 6:22 p.m.
On 06/24/10 13:59, David S. Ahern wrote:
> 
> 
> On 06/23/10 22:45, TJ wrote:
>>
>>> ---------- Forwarded message ----------
>>> From: Timothy Jones <one.timothy.jones@gmail.com>
>>> Date: Wed, Jun 23, 2010 at 9:07 PM
>>> Subject: Guest OS hangs on usb_add
>>> To: qemu-devel@nongnu.org
>>>
>>>
>>> With some digging around I found out that the qemu hangs in
>>> usb_host_claim_interfaces, which is caused by screwed up usb
>>> descriptor. The device reports the following:
>>>
>>> (gdb) p dev->descr_len
>>> $21 = 50
>>> (gdb) p /x dev->descr[0]@50
>>> $23 = {0x18, 0x1, 0x0, 0x1, 0xff, 0xff, 0xff, 0x8, 0x47, 0x46, 0x0,
>>> 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x9, 0x2, 0x20,
>>>  0x0, 0x1, 0x1, 0x0, 0x80, 0x19, 0x9, 0x4, 0x0, 0x0, 0x2, 0xff, 0xff,
>>> 0xff, 0x0, 0x7, 0x5, 0x81, 0x2, 0x40, 0x0, 0x0,
>>>  0x7, 0x5, 0x3, 0x2, 0x10, 0x0, 0x0}
>>>
>>> The first 0x18 (Device Descriptor bLength) is supposed to be decimal
>>> 18, not hex! According to USB spec, if the device reports size greater
>>> than expected, the host is supposed ignore the extra bytes. So qemu
>>> behaves correctly here. However, with this length, the following
>>> Configuration Descriptor length falls on a 0x0 and so the qemu spins
>>> in an endless loop. (This is prolly something that should be detected
>>> and reported as error by qemu.)
>>>
>>> My question is: This 0x18 -- is this something that comes from the
>>> device itself (ie, firmware bug)? Or does it come from the USB
>>> subsystem?
> 
> What kind of device is this?
> 
> David
> 

Universal Remote Control MX-950

http://www.universalremote.com/product_detail.php?model=35

-TJ
TJ - June 25, 2010, 5:23 p.m.
On 06/25/10 12:32, Gianni Tedesco wrote:
> A device MAY provide extended descriptors in 2 ways mentioned in the
> spec, but ISTR finding at least one device in the wild with standard
> descriptors extended which were not so much used by the "host" but by
> application software. So not sure about your patch, a quirks blacklist
> based on idDevice/idProduct might be the better fix here.

Makes sense. I should add vend/prod id check.

> However the more serious problem is spinning on zero length descriptor
> when truncated descriptors are not valid and zero length (in fact < 2)
> is totally unacceptable. Following patch checks for truncation.

Gianni, Please check my later patch submitted last night. I basically did the
same thing you did, but with few differences:

- if descriptor size is < 2, goto fail
- if the descriptor is USB_DT_CONFIG, we can skip through all the sub
descriptors using wTotalLength field.
- otherwise, simply skip it

One thing to also watch out for is the string descriptors. I might be wrong, but
it appears (from reading the doc) that string descriptors (at least for the
device descriptor) can be interspersed with the config descriptors, in which
case (config_descr_len < USB_DT_CONFIG_SIZE) without checking descriptor type
might unwittingly lead to failure.

-TJ

> diff --git a/hw/usb.h b/hw/usb.h
> index 00d2802..efd4a65 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -117,6 +117,14 @@
>  #define USB_DT_INTERFACE		0x04
>  #define USB_DT_ENDPOINT			0x05
>  
> +/*
> + * Descriptor sizes per descriptor type
> + */
> +#define USB_DT_DEVICE_SIZE		18
> +#define USB_DT_CONFIG_SIZE		9
> +#define USB_DT_INTERFACE_SIZE		9
> +#define USB_DT_ENDPOINT_SIZE		7
> +
>  #define USB_ENDPOINT_XFER_CONTROL	0
>  #define USB_ENDPOINT_XFER_ISOC		1
>  #define USB_ENDPOINT_XFER_BULK		2
> diff --git a/usb-linux.c b/usb-linux.c
> index 88273ff..d259290 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -299,7 +299,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>  
>      i = 0;
>      dev_descr_len = dev->descr[0];
> -    if (dev_descr_len > dev->descr_len) {
> +    if ( dev_descr_len < USB_DT_DEVICE_SIZE || dev_descr_len > dev->descr_len) {
>          goto fail;
>      }
>  
> @@ -314,6 +314,8 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>              continue;
>          }
>          config_descr_len = dev->descr[i];
> +        if ( config_descr_len < USB_DT_CONFIG_SIZE )
> +            goto fail;
>  
>          printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
>
Gianni Tedesco - June 28, 2010, 12:32 p.m.
On Fri, 2010-06-25 at 18:23 +0100, TJ wrote:
> On 06/25/10 12:32, Gianni Tedesco wrote:
> > A device MAY provide extended descriptors in 2 ways mentioned in the
> > spec, but ISTR finding at least one device in the wild with standard
> > descriptors extended which were not so much used by the "host" but by
> > application software. So not sure about your patch, a quirks blacklist
> > based on idDevice/idProduct might be the better fix here.
> 
> Makes sense. I should add vend/prod id check.
> 
> > However the more serious problem is spinning on zero length descriptor
> > when truncated descriptors are not valid and zero length (in fact < 2)
> > is totally unacceptable. Following patch checks for truncation.
> 
> Gianni, Please check my later patch submitted last night. I basically did the
> same thing you did, but with few differences:
> 
> - if descriptor size is < 2, goto fail
> - if the descriptor is USB_DT_CONFIG, we can skip through all the sub
> descriptors using wTotalLength field.
> - otherwise, simply skip it

Good point, just seen you patch and it looks good.

> One thing to also watch out for is the string descriptors. I might be wrong, but
> it appears (from reading the doc) that string descriptors (at least for the
> device descriptor) can be interspersed with the config descriptors, in which
> case (config_descr_len < USB_DT_CONFIG_SIZE) without checking descriptor type
> might unwittingly lead to failure.

Yeah definitely, descriptors can be in pretty much any old order so the
code should not rely on any of that.

FWIW, I am signing off on your approach :)

Gianni Tedesco
TJ - June 28, 2010, 2:36 p.m.
On 06/28/10 08:32, Gianni Tedesco wrote:
> 
> FWIW, I am signing off on your approach :)
> 
> Gianni Tedesco
> 

Thank you Gianni :) I am gonna add simple vend/prod id check to my 0x18 hack and
resubmit final version.

-TJ

Patch

--- hw/usb.h.orig	2010-06-23 22:55:27.649182672 -0400
+++ hw/usb.h	2010-06-23 22:56:09.937910171 -0400
@@ -117,6 +117,8 @@ 
 #define USB_DT_INTERFACE		0x04
 #define USB_DT_ENDPOINT			0x05
 
+#define USB_DT_DEVICE_LEN		18
+
 #define USB_ENDPOINT_XFER_CONTROL	0
 #define USB_ENDPOINT_XFER_ISOC		1
 #define USB_ENDPOINT_XFER_BULK		2
--- usb-linux.c.orig	2010-06-23 22:56:23.191339634 -0400
+++ usb-linux.c	2010-06-24 00:08:19.437515669 -0400
@@ -299,6 +299,9 @@ 
 
     i = 0;
     dev_descr_len = dev->descr[0];
+    if (dev_descr_len == 0x18)
+        dev_descr_len = USB_DT_DEVICE_LEN; /* for buggy device(s) reporting len in hex */
+
     if (dev_descr_len > dev->descr_len) {
         goto fail;
     }