diff mbox

Fwd: [PATCH v2] Guest OS hangs on usb_add

Message ID 4CD02560.9030103@gmail.com
State New
Headers show

Commit Message

TJ Nov. 2, 2010, 2:51 p.m. UTC 
Doesn't look like this has ever been committed. qemu-kvm-0.13 has just arrived
to the portage tree, but I am still having problems with it. I checked the git
log and it's not there! Please commit.
-TJ

-------- Original Message --------
Subject: [PATCH v2] Guest OS hangs on usb_add
Date: Mon, 28 Jun 2010 10:47:03 -0400
From: TJ <one.timothy.jones@gmail.com>
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: Gianni Tedesco <gianni.tedesco@citrix.com>

This is a small patch to sligtly "intelligentify" usb device and
config descriptor parsing and to handle bug with certain usb
device (URC MX-950) reporting device desriptor length as 0x18
instead of 18 with added vendor_id/product_id check
---
 hw/usb.h    |    5 +++++
 usb-linux.c |   37 ++++++++++++++++++++++---------------
 2 files changed, 27 insertions(+), 15 deletions(-)

     }

     if (i >= dev->descr_len) {

Comments

Anthony Liguori Nov. 16, 2010, 3 p.m. UTC | #1 
On 11/02/2010 09:51 AM, TJ wrote:
> Doesn't look like this has ever been committed. qemu-kvm-0.13 has just arrived
> to the portage tree, but I am still having problems with it. I checked the git
> log and it's not there! Please commit.
>    

One off device hacks are concerning because it's basically impossible to 
review.

Why does this work on bare metal?

Regards,

Anthony Liguori

> -TJ
>
> -------- Original Message --------
> Subject: [PATCH v2] Guest OS hangs on usb_add
> Date: Mon, 28 Jun 2010 10:47:03 -0400
> From: TJ<one.timothy.jones@gmail.com>
> To: qemu-devel@nongnu.org<qemu-devel@nongnu.org>
> CC: Gianni Tedesco<gianni.tedesco@citrix.com>
>
> This is a small patch to sligtly "intelligentify" usb device and
> config descriptor parsing and to handle bug with certain usb
> device (URC MX-950) reporting device desriptor length as 0x18
> instead of 18 with added vendor_id/product_id check
> ---
>   hw/usb.h    |    5 +++++
>   usb-linux.c |   37 ++++++++++++++++++++++---------------
>   2 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/hw/usb.h b/hw/usb.h
> index 00d2802..5c3528f 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -117,6 +117,11 @@
>   #define USB_DT_INTERFACE		0x04
>   #define USB_DT_ENDPOINT			0x05
>
> +#define USB_DT_DEVICE_LEN		18
> +#define USB_DT_CONFIG_LEN		9
> +#define USB_DT_INTERFACE_LEN		9
> +#define USB_DT_ENDPOINT_LEN		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..2ac6562 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -288,7 +288,7 @@ static void async_cancel(USBPacket *unused, void *opaque)
>
>   static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>   {
> -    int dev_descr_len, config_descr_len;
> +    int dev_descr_len, config_descr_total_len;
>       int interface, nb_interfaces;
>       int ret, i;
>
> @@ -297,32 +297,39 @@ static int usb_host_claim_interfaces(USBHostDevice *dev,
> int configuration)
>
>       DPRINTF("husb: claiming interfaces. config %d\n", configuration);
>
> -    i = 0;
>       dev_descr_len = dev->descr[0];
> -    if (dev_descr_len>  dev->descr_len) {
> +    if (dev_descr_len == 0x18&&  dev->descr[ 8] == 0x47&&  dev->descr[ 9] == 0x46
> +&&  dev->descr[10] == 0x00&&  dev->descr[11] == 0x30)
> +        dev_descr_len = USB_DT_DEVICE_LEN; /* for buggy MX-950 remote reporting
> len in hex */
> +
> +    if (dev_descr_len>  dev->descr_len || dev_descr_len<  USB_DT_DEVICE_LEN ||
> dev->descr[1] != USB_DT_DEVICE) {
> +        fprintf(stderr, "husb: invalid device descriptor\n");
>           goto fail;
>       }
>
> -    i += dev_descr_len;
> -    while (i<  dev->descr_len) {
> +    for (i = dev_descr_len; i<  dev->descr_len; ) {
>           DPRINTF("husb: i is %d, descr_len is %d, dl %d, dt %d\n",
>                   i, dev->descr_len,
>                  dev->descr[i], dev->descr[i+1]);
>
> -        if (dev->descr[i+1] != USB_DT_CONFIG) {
> -            i += dev->descr[i];
> -            continue;
> +        if (dev->descr[i]<  2) {
> +            fprintf(stderr, "husb: invalid descriptor\n");
> +            goto fail;
>           }
> -        config_descr_len = dev->descr[i];
> +        if (dev->descr[i+1] == USB_DT_CONFIG) {
> +            config_descr_total_len = dev->descr[i+2] + (dev->descr[i+3]<<  8);
>
> -        printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
> +            printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
>
> -        if (configuration<  0 || configuration == dev->descr[i + 5]) {
> -            configuration = dev->descr[i + 5];
> -            break;
> -        }
> +            if (configuration<  0 || configuration == dev->descr[i + 5]) {
> +                configuration = dev->descr[i + 5];
> +                break;
> +            }
>
> -        i += config_descr_len;
> +            i += config_descr_total_len;
> +        }
> +        else
> +            i += dev->descr[i];
>       }
>
>       if (i>= dev->descr_len) {
>
>
>
TJ Nov. 16, 2010, 7:36 p.m. UTC | #2 
On 11/16/2010 10:00 AM, Anthony Liguori wrote:
> On 11/02/2010 09:51 AM, TJ wrote:
>> Doesn't look like this has ever been committed. qemu-kvm-0.13 has just arrived
>> to the portage tree, but I am still having problems with it. I checked the git
>> log and it's not there! Please commit.
>>    
> 
> One off device hacks are concerning because it's basically impossible to review.
> 
> Why does this work on bare metal?
> 
> Regards,
> 
> Anthony Liguori
> 

Probably because bare metal USB 2.0 controllers don't give a damn about USB 3
spec. :)

My guess is that they ignore the device descriptor length and assume that it's
always equal 18. Although the USB 2.0 spec doesn't explicitly say anywhere that
it can't be more than 18. IIRC USB 3 even adds some extensions to the device
descriptor. And since I wanted my code to be portable and USB 3 ready ;) I rely
on the value in dev_descr_len.

BTW, this patch is more than just a hack for the device in question. Without
this patch qemu simply locks up when I attach the remote and spins in endless
loop, because USB parsing is so very primitive. With this patch, USB parsing is
done more intelligently and devices with whacky USB descriptors are simply rejected.

The hack part is really just 3 lines:

>> +    if (dev_descr_len == 0x18 && dev->descr[ 8] == 0x47 && dev->descr[ 9] == 0x46
>> +                              && dev->descr[10] == 0x00 && dev->descr[11] == 0x30)
>> +        dev_descr_len = USB_DT_DEVICE_LEN; /* for buggy MX-950 remote reporting len in hex */

And it is very harmless, as all it does is overwrites the device descriptor
length with correct one.

If you don't like the hack, you can just remove the 3 lines above and use the
rest of the patch. I will just have to remember to manually patch mine every
time I upgrade.

Your thoughts?

-TJ
Anthony Liguori Nov. 16, 2010, 7:55 p.m. UTC | #3 
On 11/16/2010 01:36 PM, TJ wrote:
> On 11/16/2010 10:00 AM, Anthony Liguori wrote:
>    
>> On 11/02/2010 09:51 AM, TJ wrote:
>>      
>>> Doesn't look like this has ever been committed. qemu-kvm-0.13 has just arrived
>>> to the portage tree, but I am still having problems with it. I checked the git
>>> log and it's not there! Please commit.
>>>
>>>        
>> One off device hacks are concerning because it's basically impossible to review.
>>
>> Why does this work on bare metal?
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>      
> Probably because bare metal USB 2.0 controllers don't give a damn about USB 3
> spec. :)
>
> My guess is that they ignore the device descriptor length and assume that it's
> always equal 18. Although the USB 2.0 spec doesn't explicitly say anywhere that
> it can't be more than 18. IIRC USB 3 even adds some extensions to the device
> descriptor. And since I wanted my code to be portable and USB 3 ready ;) I rely
> on the value in dev_descr_len.
>
> BTW, this patch is more than just a hack for the device in question. Without
> this patch qemu simply locks up when I attach the remote and spins in endless
> loop, because USB parsing is so very primitive. With this patch, USB parsing is
> done more intelligently and devices with whacky USB descriptors are simply rejected.
>
> The hack part is really just 3 lines:
>
>    
>>> +    if (dev_descr_len == 0x18&&  dev->descr[ 8] == 0x47&&  dev->descr[ 9] == 0x46
>>> +&&  dev->descr[10] == 0x00&&  dev->descr[11] == 0x30)
>>> +        dev_descr_len = USB_DT_DEVICE_LEN; /* for buggy MX-950 remote reporting len in hex */
>>>        
> And it is very harmless, as all it does is overwrites the device descriptor
> length with correct one.
>
> If you don't like the hack, you can just remove the 3 lines above and use the
> rest of the patch. I will just have to remember to manually patch mine every
> time I upgrade.
>
> Your thoughts?
>    

Yeah, that bit is a bit too gnarly for my tastes, but if you can resend 
the rest of it with a Signed-off-by, I'd appreciate.

Regards,

Anthony Liguori

> -TJ
>
TJ Feb. 23, 2011, 1:20 a.m. UTC | #4 
On 11/16/2010 02:55 PM, Anthony Liguori wrote:
>>
>> If you don't like the hack, you can just remove the 3 lines above and use the
>> rest of the patch. I will just have to remember to manually patch mine every
>> time I upgrade.
>>
>> Your thoughts?
>>    
> 
> Yeah, that bit is a bit too gnarly for my tastes, but if you can resend the rest
> of it with a Signed-off-by, I'd appreciate.
> 
> Regards,
> 
> Anthony Liguori
> 
>> -TJ
>>    
> 

Antonio,
Did anything come of this? I resubmitted the patch without the hack, but it
don't look like it was committed:

http://lists.nongnu.org/archive/html/qemu-devel/2010-11/msg01496.html

Ciao,
TJ
diff mbox

Patch

diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..5c3528f 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -117,6 +117,11 @@ 
 #define USB_DT_INTERFACE		0x04
 #define USB_DT_ENDPOINT			0x05

+#define USB_DT_DEVICE_LEN		18
+#define USB_DT_CONFIG_LEN		9
+#define USB_DT_INTERFACE_LEN		9
+#define USB_DT_ENDPOINT_LEN		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..2ac6562 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -288,7 +288,7 @@  static void async_cancel(USBPacket *unused, void *opaque)

 static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
 {
-    int dev_descr_len, config_descr_len;
+    int dev_descr_len, config_descr_total_len;
     int interface, nb_interfaces;
     int ret, i;

@@ -297,32 +297,39 @@  static int usb_host_claim_interfaces(USBHostDevice *dev,
int configuration)

     DPRINTF("husb: claiming interfaces. config %d\n", configuration);

-    i = 0;
     dev_descr_len = dev->descr[0];
-    if (dev_descr_len > dev->descr_len) {
+    if (dev_descr_len == 0x18 && dev->descr[ 8] == 0x47 && dev->descr[ 9] == 0x46
+                              && dev->descr[10] == 0x00 && dev->descr[11] == 0x30)
+        dev_descr_len = USB_DT_DEVICE_LEN; /* for buggy MX-950 remote reporting
len in hex */
+
+    if (dev_descr_len > dev->descr_len || dev_descr_len < USB_DT_DEVICE_LEN ||
dev->descr[1] != USB_DT_DEVICE) {
+        fprintf(stderr, "husb: invalid device descriptor\n");
         goto fail;
     }

-    i += dev_descr_len;
-    while (i < dev->descr_len) {
+    for (i = dev_descr_len; i < dev->descr_len; ) {
         DPRINTF("husb: i is %d, descr_len is %d, dl %d, dt %d\n",
                 i, dev->descr_len,
                dev->descr[i], dev->descr[i+1]);

-        if (dev->descr[i+1] != USB_DT_CONFIG) {
-            i += dev->descr[i];
-            continue;
+        if (dev->descr[i] < 2) {
+            fprintf(stderr, "husb: invalid descriptor\n");
+            goto fail;
         }
-        config_descr_len = dev->descr[i];
+        if (dev->descr[i+1] == USB_DT_CONFIG) {
+            config_descr_total_len = dev->descr[i+2] + (dev->descr[i+3] << 8);

-        printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
+            printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);

-        if (configuration < 0 || configuration == dev->descr[i + 5]) {
-            configuration = dev->descr[i + 5];
-            break;
-        }
+            if (configuration < 0 || configuration == dev->descr[i + 5]) {
+                configuration = dev->descr[i + 5];
+                break;
+            }

-        i += config_descr_len;
+            i += config_descr_total_len;
+        }
+        else
+            i += dev->descr[i];