[U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration

[Albert ARIBAUD]

Hi Julius,

On Wed, 17 Jul 2013 17:55:19 -0700, Julius Werner
<jwerner at chromium.org> wrote:

> 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 at chromium.org>
> ---

Please do not forget patch history.

>  common/usb.c     | 73 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  common/usb_hub.c | 14 ++++-------
>  2 files changed, 64 insertions(+), 23 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 55fff5b..ab7bafe 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,39 @@ 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));
> +	if (buffer[0] != USB_DT_CONFIG_SIZE)
> +		printf("Ignoring invalid USB CFG length (%d)\n", buffer[0]);
> +	memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE);
>  	dev->config.no_of_if = 0;

From a security / robustness standpoint,

- if the descriptor length field is found to be abnormal, then the code
  should not process the packet at all. Here it seems it only warns
  then goes on to use the descriptor.

- if for some reason (although I don't see any) the descriptor must be
  processed despite its wrong length, then only the bytes at positions
  0..length-1 should be considered; bytes beyond the descriptor's
  announced length should be deemed undefined (and, I might add,
  should not be even be read from the device).

>  	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) {

Does this change fall into either "using the well-defined length"
or "adding safety to out-of-order descriptors" category? If none, then
there should be a third category of change documented in the commit
message.

>  		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]);
> +				if (buffer[index] != USB_DT_INTERFACE_SIZE)
> +					printf("Ignoring invalid USB IF length"
> +						" (%d)\n", buffer[index]);
> +				memcpy(if_desc, &buffer[index],
> +					USB_DT_INTERFACE_SIZE);

Again here, a wrong length is detected yet the descriptor is used.

>  				if_desc->no_of_ep = 0;
>  				if_desc->num_altsetting = 1;
>  				curr_if_num =
> @@ -392,12 +408,29 @@ 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++;
> +			if (buffer[index] != USB_DT_ENDPOINT_SIZE)
> +				printf("Ignoring invalid USB EP length (%d)\n",
> +					buffer[index]);

Ditto.

>  			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 +443,21 @@ 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("EPC descriptor out of order!\n");
> +				break;
> +			}
>  			if_desc = &dev->config.if_desc[ifno];
> +			if (buffer[index] != USB_DT_SS_EP_COMP_SIZE)
> +				printf("Ignoring invalid USB EPC length (%d)\n",
> +					buffer[index]);

Ditto.

>  			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 +536,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 +550,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);

This message seems less clear than the one it replaces; I suspect
"failed to get hub descriptor 2nd giving up" is not a valid sentence.

>  		return -1;
>  	}
> -	memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength);
> +	memcpy((unsigned char *)&hub->desc, buffer, length);

I'm fine with this change here, as it does just what's required:
limiting the memcpy() to the receiving buffer size. This IMO is
what should be done also in the other memcpy()s.

>  	/* adjust 16bit values */
>  	put_unaligned(le16_to_cpu(get_unaligned(
>  			&descriptor->wHubCharacteristics)),

Amicalement,
-- 
Albert.