The existing USB configuration parsing code relies on the descriptors' own length values when reading through the configuration blob. Since the size of those descriptors is always well-defined, we should rather use the known sizes instead of trusting device-provided values to be correct. Also adds some safety to potential out-of-order descriptors.

Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 Signed-off-by: Julius Werner jwerner@chromium.org --- common/usb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++------------ common/usb_hub.c | 14 ++++--------- 2 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/common/usb.c b/common/usb.c index 55fff5b..4cf1d7a 100644 --- a/common/usb.c +++ b/common/usb.c @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) /******************************************************************************* * Parse the config, located in buffer, and fills the dev->config structure. * Note that all little/big endian swapping are done automatically. + * (wTotalLength has already been swapped when it was read.) */ static int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) @@ -361,24 +362,34 @@ static int usb_parse_config(struct usb_device *dev, head->bDescriptorType); return -1; } - memcpy(&dev->config, buffer, buffer[0]); - le16_to_cpus(&(dev->config.desc.wTotalLength)); + memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE); dev->config.no_of_if = 0;

index = dev->config.desc.bLength; /* Ok the first entry must be a configuration entry, * now process the others */ head = (struct usb_descriptor_header *) &buffer[index]; - while (index + 1 < dev->config.desc.wTotalLength) { + while (index + 1 < dev->config.desc.wTotalLength && head->bLength) { switch (head->bDescriptorType) { case USB_DT_INTERFACE: + if (index + USB_DT_INTERFACE_SIZE > + dev->config.desc.wTotalLength) { + puts("USB IF descriptor overflowed buffer!\n"); + break; + } if (((struct usb_interface_descriptor *) \ &buffer[index])->bInterfaceNumber != curr_if_num) { /* this is a new interface, copy new desc */ ifno = dev->config.no_of_if; + if (ifno >= USB_MAXINTERFACES) { + puts("Too many USB interfaces!\n"); + /* try to go on with what we have */ + return 1; + } if_desc = &dev->config.if_desc[ifno]; dev->config.no_of_if++; - memcpy(if_desc, &buffer[index], buffer[index]); + memcpy(if_desc, &buffer[index], + USB_DT_INTERFACE_SIZE); if_desc->no_of_ep = 0; if_desc->num_altsetting = 1; curr_if_num = @@ -392,12 +403,26 @@ static int usb_parse_config(struct usb_device *dev, } break; case USB_DT_ENDPOINT: + if (index + USB_DT_ENDPOINT_SIZE > + dev->config.desc.wTotalLength) { + puts("USB EP descriptor overflowed buffer!\n"); + break; + } + if (ifno < 0) { + puts("Endpoint descriptor out of order!\n"); + break; + } epno = dev->config.if_desc[ifno].no_of_ep; if_desc = &dev->config.if_desc[ifno]; + if (epno > USB_MAXENDPOINTS) { + printf("Interface %d has too many endpoints!\n", + if_desc->desc.bInterfaceNumber); + return 1; + } /* found an endpoint */ if_desc->no_of_ep++; memcpy(&if_desc->ep_desc[epno], - &buffer[index], buffer[index]); + &buffer[index], USB_DT_ENDPOINT_SIZE); ep_wMaxPacketSize = get_unaligned(&dev->config.\ if_desc[ifno].\ ep_desc[epno].\ @@ -410,9 +435,18 @@ static int usb_parse_config(struct usb_device *dev, debug("if %d, ep %d\n", ifno, epno); break; case USB_DT_SS_ENDPOINT_COMP: + if (index + USB_DT_SS_EP_COMP_SIZE > + dev->config.desc.wTotalLength) { + puts("USB EPC descriptor overflowed buffer!\n"); + break; + } + if (ifno < 0 || epno < 0) { + puts("EP SS descriptor out of order!\n"); + break; + } if_desc = &dev->config.if_desc[ifno]; memcpy(&if_desc->ss_ep_comp_desc[epno], - &buffer[index], buffer[index]); + &buffer[index], USB_DT_SS_EP_COMP_SIZE); break; default: if (head->bLength == 0) @@ -491,7 +525,7 @@ int usb_get_configuration_no(struct usb_device *dev, unsigned char *buffer, int cfgno) { int result; - unsigned int tmp; + unsigned int length; struct usb_config_descriptor *config;

config = (struct usb_config_descriptor *)&buffer[0]; @@ -505,16 +539,18 @@ int usb_get_configuration_no(struct usb_device *dev, "(expected %i, got %i)\n", 9, result); return -1; } - tmp = le16_to_cpu(config->wTotalLength); + length = le16_to_cpu(config->wTotalLength);

- if (tmp > USB_BUFSIZ) { - printf("usb_get_configuration_no: failed to get " \ - "descriptor - too long: %d\n", tmp); + if (length > USB_BUFSIZ) { + printf("%s: failed to get descriptor - too long: %d\n", + __func__, length); return -1; }

- result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, tmp); - debug("get_conf_no %d Result %d, wLength %d\n", cfgno, result, tmp); + result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, length); + debug("get_conf_no %d Result %d, wLength %d\n", cfgno, result, length); + config->wTotalLength = length; /* validated, with CPU byte order */ + return result; }

diff --git a/common/usb_hub.c b/common/usb_hub.c index 774ba63..d30962e 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -320,7 +320,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)

static int usb_hub_configure(struct usb_device *dev) { - int i; + int i, length; ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); unsigned char *bitmap; short hubCharacteristics; @@ -341,20 +341,14 @@ static int usb_hub_configure(struct usb_device *dev) } descriptor = (struct usb_hub_descriptor *)buffer;

- /* silence compiler warning if USB_BUFSIZ is > 256 [= sizeof(char)] */ - i = descriptor->bLength; - if (i > USB_BUFSIZ) { - debug("usb_hub_configure: failed to get hub " \ - "descriptor - too long: %d\n", descriptor->bLength); - return -1; - } + length = min(descriptor->bLength, sizeof(struct usb_hub_descriptor));

- if (usb_get_hub_descriptor(dev, buffer, descriptor->bLength) < 0) { + if (usb_get_hub_descriptor(dev, buffer, length) < 0) { debug("usb_hub_configure: failed to get hub " \ "descriptor 2nd giving up %lX\n", dev->status); return -1; } - memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength); + memcpy((unsigned char *)&hub->desc, buffer, length); /* adjust 16bit values */ put_unaligned(le16_to_cpu(get_unaligned( &descriptor->wHubCharacteristics)),