Re: [Qemu-devel] Guest OS hangs on usb_add
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
Re: [Qemu-devel] Guest OS hangs on usb_add
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
Re: [Qemu-devel] Guest OS hangs on usb_add
On Thu, 2010-06-24 at 05:45 +0100, TJ wrote:
> 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.
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.
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.
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_ENDPOINT0x05
+/*
+ * 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);
Re: [Qemu-devel] Guest OS hangs on usb_add
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_SIZE9
> +#define USB_DT_ENDPOINT_SIZE 7
> +
> #define USB_ENDPOINT_XFER_CONTROL0
> #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);
>
Re: [Qemu-devel] Guest OS hangs on usb_add
On 06/24/10 02:42, Markus Armbruster wrote: > A botched up patch is often a pretty effective way to get somebody to > fix the thing correctly. OK, I gave it a shot and sent it to the list. Shoulda prolly added a disclaimer in case it blows something up ;) -TJ
Re: [Qemu-devel] Guest OS hangs on usb_add
On 06/24/10 13:59, David S. Ahern wrote:
>
>
> On 06/23/10 22:45, TJ wrote:
>>
>>> -- Forwarded message --
>>> From: Timothy Jones
>>> 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...@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
Re: [Qemu-devel] Guest OS hangs on usb_add
On 06/23/10 22:45, TJ wrote:
>
>> -- Forwarded message --
>> From: Timothy Jones
>> 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...@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
Re: [Qemu-devel] Guest OS hangs on usb_add
Timothy Jones writes:
> 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...@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.)
Yup.
> 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?
It's been a while since I hacked USB, but IIRC it's from the device.
> 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?
A botched up patch is often a pretty effective way to get somebody to
fix the thing correctly.
[Qemu-devel] Guest OS hangs on usb_add
> -- Forwarded message --
> From: Timothy Jones
> 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...@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
--- hw/usb.h.orig 2010-06-23 22:55:27.649182672 -0400
+++ hw/usb.h2010-06-23 22:56:09.937910171 -0400
@@ -117,6 +117,8 @@
#define USB_DT_INTERFACE 0x04
#define USB_DT_ENDPOINT0x05
+#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.orig2010-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;
}
[Qemu-devel] Guest OS hangs on usb_add
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...@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
-- Forwarded message --
From: Timothy Jones
Date: Wed, Jun 23, 2010 at 2:21 PM
Subject: Guest OS hangs on usb_add
To: qemu-devel@nongnu.org
I am trying to attach universal remote control (URC MX-950) to Windows
XP guest as follows:
==
(qemu) info usbhost
Device 1.1, speed 480 Mb/s
Hub: USB device 1d6b:0002, EHCI Host Controller
Device 2.1, speed 480 Mb/s
Hub: USB device 1d6b:0002, EHCI Host Controller
Device 1.2, speed 480 Mb/s
Hub: USB device 8087:0020
Device 2.2, speed 480 Mb/s
Hub: USB device 8087:0020
Device 1.3, speed 480 Mb/s
Class ef: USB device 0c45:6416, Laptop_Integrated_Webcam_2M
Device 2.5, speed 12 Mb/s
Vendor Specific: USB device 4647:3000 < MX-950 USB remote
Device 2.4, speed 1.5 Mb/s
Class 00: USB device 05ac:0304, Apple Optical USB Mouse
(qemu) usb_add host:4647:3000
usb_create: no bus specified, using "usb.0" for "usb-host"
husb: open device 2.5
==
The guest just freezes including the monitor and pegs one of the cores
on host OS at 100%.
I tried the same with 2 other devices on the above list (Apple Mouse
and Webcam) and both worked fine.
I am running: (qemu) info version
0.12.50 (qemu-kvm-devel)
Host: Linux studio 2.6.34-gentoo-r1 #2 SMP Tue Jun 22 23:21:18 EDT
2010 x86_64 Intel(R) Core(TM) i5 CPU M 520 @ 2.40GHz GenuineIntel
GNU/Linux
Guest: Windows XP Pro SP3
FWIW, I tried the above with and without kvm and kvm-intel modules loaded.
Anybody know what could be causing this?
Thank you.
-TJ
[Qemu-devel] Guest OS hangs on usb_add
I am trying to attach universal remote control (URC MX-950) to Windows XP guest as follows: == (qemu) info usbhost Device 1.1, speed 480 Mb/s Hub: USB device 1d6b:0002, EHCI Host Controller Device 2.1, speed 480 Mb/s Hub: USB device 1d6b:0002, EHCI Host Controller Device 1.2, speed 480 Mb/s Hub: USB device 8087:0020 Device 2.2, speed 480 Mb/s Hub: USB device 8087:0020 Device 1.3, speed 480 Mb/s Class ef: USB device 0c45:6416, Laptop_Integrated_Webcam_2M Device 2.5, speed 12 Mb/s Vendor Specific: USB device 4647:3000 < MX-950 USB remote Device 2.4, speed 1.5 Mb/s Class 00: USB device 05ac:0304, Apple Optical USB Mouse (qemu) usb_add host:4647:3000 usb_create: no bus specified, using "usb.0" for "usb-host" husb: open device 2.5 == The guest just freezes including the monitor and pegs one of the cores on host OS at 100%. I tried the same with 2 other devices on the above list (Apple Mouse and Webcam) and both worked fine. I am running: (qemu) info version 0.12.50 (qemu-kvm-devel) Host: Linux studio 2.6.34-gentoo-r1 #2 SMP Tue Jun 22 23:21:18 EDT 2010 x86_64 Intel(R) Core(TM) i5 CPU M 520 @ 2.40GHz GenuineIntel GNU/Linux Guest: Windows XP Pro SP3 FWIW, I tried the above with and without kvm and kvm-intel modules loaded. Anybody know what could be causing this? Thank you. -TJ
