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

[Marek Vasut <marex@denx.de>]

Dear Julius Werner,

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

I finally got time to properly look into the patch, sorry for the delay.

>  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]);

Mulling over this some more, I suspect if the device does have incorrect config 
descriptor, we should just ignore the device because it's broken piece of junk.

> +	memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE);
>  	dev->config.no_of_if = 0;

Looking at this, the memcpy is incorrect in the first place. It shouldn't memcpy 
into dev->config, but into dev->config.desc . And in turn, you an do

memcpy(&dev->config.desc, buffer, sizeof(dev->config.desc));

Which is even better, since you always take into account the size of the 
structure member. This would be worth fixing the right way.

Looking through the usage of these structures, you opened quite the ugly can of 
worms here :(

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

Urgh, who the heck wrote this code. Damn that's a mess. Anyway, I suspect this 
might still explode if we go over USB_BUFSIZ in wTotalLength . Worth fixing.

>  		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;
> +			}

Can you maybe use sizeof(struct usb_interface_descriptor) ? As a future 
proposal, we might really want to get rid of the USB_DT_INTERFACE_SIZE in favor 
of using sizeof(), that'd be much less error prone.

>  			if (((struct usb_interface_descriptor *) \
>  			     &buffer[index])->bInterfaceNumber != curr_if_num) {

Would be nice to clean this up into "understandable" format by defining a 
variable for the &buffer[index] and than just simply comparing this var-
>bInterfaceNumber and 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;
> +				}

OK

>  				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]);

Let's just ignore the descriptor if it's incorrect.

> +				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 +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;
> +			}

See my rambling above, otherwise I agree.

>  			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]);
>  			memcpy(&if_desc->ep_desc[epno],
> -				&buffer[index], buffer[index]);
> +				&buffer[index], USB_DT_ENDPOINT_SIZE);

Again, sizeof() ?

>  			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]);
>  			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 */

The above assignment is new, why ?

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

OK