| Message ID | 4C22E300.5020109@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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); >
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
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
--- 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; }