Skip to contentSkip to conversation

usb: host-linux: Ignore parsing errors of the device descriptors

PATCH In review Jan Kiszka 2 participants 4 messages Nov 15, 2012

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 usb: host-linux: Ignore parsing errors of the device descriptors

The Linux is more tolerant here as well: Just stop parsing the device descriptors when an error is detected but do not reset what was found so far. This allows to run buggy devices with partially invalid descriptors.

---
 hw/usb/host-linux.c |   31 +++++++++++--------------------
 1 files changed, 11 insertions(+), 20 deletions(-)
hw/usb/host-linux.c +11 -20
index 3a258b4..b8127e1 100644
@@ -135,7 +135,7 @@ static int parse_filter(const char *spec, struct USBAutoFilter *f);
 static void usb_host_auto_check(void *unused);
 static int usb_host_read_file(char *line, size_t line_size,
                             const char *device_file, const char *device_name);
-static int usb_linux_update_endp_table(USBHostDevice *s);
+static void usb_linux_update_endp_table(USBHostDevice *s);
 
 static int usb_host_usbfs_type(USBHostDevice *s, USBPacket *p)
 {
@@ -1122,8 +1122,7 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
     return USB_RET_ASYNC;
 }
 
-/* returns 1 on problem encountered or 0 for success */
-static int usb_linux_update_endp_table(USBHostDevice *s)
+static void usb_linux_update_endp_table(USBHostDevice *s)
 {
     static const char *tname[] = {
         [USB_ENDPOINT_XFER_CONTROL] = "control",
@@ -1149,23 +1148,23 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
         if (d->bLength < 2) {
             trace_usb_host_parse_error(s->bus_num, s->addr,
                                        "descriptor too short");
-            goto error;
+            return;
         }
         if (i + d->bLength > s->descr_len) {
             trace_usb_host_parse_error(s->bus_num, s->addr,
                                        "descriptor too long");
-            goto error;
+            return;
         }
         switch (d->bDescriptorType) {
         case 0:
             trace_usb_host_parse_error(s->bus_num, s->addr,
                                        "invalid descriptor type");
-            goto error;
+            return;
         case USB_DT_DEVICE:
             if (d->bLength < 0x12) {
                 trace_usb_host_parse_error(s->bus_num, s->addr,
                                            "device descriptor too short");
-                goto error;
+                return;
             }
             v = (d->u.device.idVendor_hi << 8) | d->u.device.idVendor_lo;
             p = (d->u.device.idProduct_hi << 8) | d->u.device.idProduct_lo;
@@ -1175,7 +1174,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             if (d->bLength < 0x09) {
                 trace_usb_host_parse_error(s->bus_num, s->addr,
                                            "config descriptor too short");
-                goto error;
+                return;
             }
             configuration = d->u.config.bConfigurationValue;
             active = (configuration == s->dev.configuration);
@@ -1186,7 +1185,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             if (d->bLength < 0x09) {
                 trace_usb_host_parse_error(s->bus_num, s->addr,
                                            "interface descriptor too short");
-                goto error;
+                return;
             }
             interface = d->u.interface.bInterfaceNumber;
             altsetting = d->u.interface.bAlternateSetting;
@@ -1199,7 +1198,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             if (d->bLength < 0x07) {
                 trace_usb_host_parse_error(s->bus_num, s->addr,
                                            "endpoint descriptor too short");
-                goto error;
+                return;
             }
             devep = d->u.endpoint.bEndpointAddress;
             pid = (devep & USB_DIR_IN) ? USB_TOKEN_IN : USB_TOKEN_OUT;
@@ -1207,7 +1206,7 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             if (ep == 0) {
                 trace_usb_host_parse_error(s->bus_num, s->addr,
                                            "invalid endpoint address");
-                goto error;
+                return;
             }
 
             type = d->u.endpoint.bmAttributes & 0x3;
@@ -1240,11 +1239,6 @@ static int usb_linux_update_endp_table(USBHostDevice *s)
             break;
         }
     }
-    return 0;
-
-error:
-    usb_ep_reset(&s->dev);
-    return 1;
 }
 
 /*
@@ -1331,10 +1325,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
     }
 
     usb_ep_init(&dev->dev);
-    ret = usb_linux_update_endp_table(dev);
-    if (ret) {
-        goto fail;
-    }
+    usb_linux_update_endp_table(dev);
 
     if (speed == -1) {
         struct usbdevfs_connectinfo ci;
Signature · 1 line
1.7.3.4
#1 lore raw

hw/usb/host-linux.c static int usb_linux_update_endp_table(USBHostDevice *s) · line 1247

Quoted 8 lines · Jan Kiszka

The Linux is more tolerant here as well: Just stop parsing the device descriptors when an error is detected but do not reset what was found so far. This allows to run buggy devices with partially invalid descriptors.

hw/usb/host-linux.c +0 -3
-error:
-    usb_ep_reset(&s->dev);
-    return 1;

I'd prefer to keep the error jump target to handle the parse error here. Dumping the reset there is fine with me, but I'd prefer this event being logged (trace point or stderr message or both) to ease trouble shooting in case a device doesn't behave as expected.

thanks,
Gerd

author
#2 lore raw
Quoted 6 lines · Gerd Hoffmann
Quoted 4 lines · Jan Kiszka

The Linux is more tolerant here as well: Just stop parsing the device descriptors when an error is detected but do not reset what was found so far. This allows to run buggy devices with partially invalid descriptors.

Quoted 8 lines

hw/usb/host-linux.c static int usb_linux_update_endp_table(USBHostDevice *s) · line 1247

Quoted 3 lines
hw/usb/host-linux.c +0 -3
-error:
-    usb_ep_reset(&s->dev);
-    return 1;

I'd prefer to keep the error jump target to handle the parse error here. Dumping the reset there is fine with me, but I'd prefer this event being logged (trace point or stderr message or both) to ease trouble shooting in case a device doesn't behave as expected.

That would be "over-logging" as we already record the individual reasons. There is simply no code to jump to after the refactoring.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#3 lore raw
Quoted 6 lines · Jan Kiszka
Quoted 4 lines
Quoted 4 lines

I'd prefer to keep the error jump target to handle the parse error here. Dumping the reset there is fine with me, but I'd prefer this event being logged (trace point or stderr message or both) to ease trouble shooting in case a device doesn't behave as expected.

That would be "over-logging" as we already record the individual reasons. There is simply no code to jump to after the refactoring.

Oh, right, we trace that already. Ok then.

Patch added to usb patch queue.

thanks,
Gerd