Patchwork [U-Boot] usb: fallback safely when a configuration descriptor is too large

login
register
mail settings
Submitter Vincent Palatin
Date July 24, 2012, 5:12 p.m.
Message ID <1343149922-17939-1-git-send-email-vpalatin@chromium.org>
Download mbox | patch
Permalink /patch/173025/
State Accepted
Delegated to: Marek Vasut
Headers show

Comments

Vincent Palatin - July 24, 2012, 5:12 p.m.
When a USB configuration descriptor was larger than our USB buffer
(512 bytes), we were skipping the full descriptor reading but then we
were still parsing and using it, triggering memory corruptions.
Now in that case, it just skips this device enumeration and displays the
appropriate message to the user, so he can fix the buffer if he wants.

This bug was triggered by some UVC webcams which have very large
configuration descriptors (e.g. a couple of kB) describing all their
supported video encodings.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
---
 common/usb.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)
Simon Glass - July 25, 2012, 7:46 a.m.
Hi Vincent,

On Tue, Jul 24, 2012 at 6:12 PM, Vincent Palatin <vpalatin@chromium.org> wrote:
> When a USB configuration descriptor was larger than our USB buffer
> (512 bytes), we were skipping the full descriptor reading but then we
> were still parsing and using it, triggering memory corruptions.
> Now in that case, it just skips this device enumeration and displays the
> appropriate message to the user, so he can fix the buffer if he wants.
>
> This bug was triggered by some UVC webcams which have very large
> configuration descriptors (e.g. a couple of kB) describing all their
> supported video encodings.
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>  common/usb.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 63a11c8..cfa1ad7 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -485,8 +485,8 @@ int usb_get_configuration_no(struct usb_device *dev,
>         tmp = le16_to_cpu(config->wTotalLength);
>
>         if (tmp > USB_BUFSIZ) {
> -               USB_PRINTF("usb_get_configuration_no: failed to get " \
> -                          "descriptor - too long: %d\n", tmp);
> +               printf("usb_get_configuration_no: failed to get " \
> +                      "descriptor - too long: %d\n", tmp);

Should this (and the one below) be USB_PRINTF() instead?

>                 return -1;
>         }
>
> @@ -921,7 +921,13 @@ int usb_new_device(struct usb_device *dev)
>         le16_to_cpus(&dev->descriptor.idProduct);
>         le16_to_cpus(&dev->descriptor.bcdDevice);
>         /* only support for one config for now */
> -       usb_get_configuration_no(dev, &tmpbuf[0], 0);
> +       err = usb_get_configuration_no(dev, &tmpbuf[0], 0);
> +       if (err < 0) {
> +               printf("usb_new_device: Cannot read configuration, " \
> +                      "skipping device %04x:%04x\n",
> +                      dev->descriptor.idVendor, dev->descriptor.idProduct);
> +               return -1;
> +       }
>         usb_parse_config(dev, &tmpbuf[0], 0);
>         usb_set_maxpacket(dev);
>         /* we set the default configuration here */
> --
> 1.7.7.3
>

Regards,
Simon

Patch

diff --git a/common/usb.c b/common/usb.c
index 63a11c8..cfa1ad7 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -485,8 +485,8 @@  int usb_get_configuration_no(struct usb_device *dev,
 	tmp = le16_to_cpu(config->wTotalLength);
 
 	if (tmp > USB_BUFSIZ) {
-		USB_PRINTF("usb_get_configuration_no: failed to get " \
-			   "descriptor - too long: %d\n", tmp);
+		printf("usb_get_configuration_no: failed to get " \
+		       "descriptor - too long: %d\n", tmp);
 		return -1;
 	}
 
@@ -921,7 +921,13 @@  int usb_new_device(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 	/* only support for one config for now */
-	usb_get_configuration_no(dev, &tmpbuf[0], 0);
+	err = usb_get_configuration_no(dev, &tmpbuf[0], 0);
+	if (err < 0) {
+		printf("usb_new_device: Cannot read configuration, " \
+		       "skipping device %04x:%04x\n",
+		       dev->descriptor.idVendor, dev->descriptor.idProduct);
+		return -1;
+	}
 	usb_parse_config(dev, &tmpbuf[0], 0);
 	usb_set_maxpacket(dev);
 	/* we set the default configuration here */