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

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

[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>
---
 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;
 
 	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]);
+				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);
 				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]);
 			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]);
 			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);
 		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)),
-- 
1.7.12.4

[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@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@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.

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

[Julius Werner]

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

Weren't you the guy who was so worried about poor Chinese devices who
don't follow the spec just two mails ago? This code just tries to make
the most of a broken response while keeping the host safe. Since the
descriptors are just put into a structure and left there for the
driver to decide which ones it's interested in anyway, more is usually
better than less.

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

This is not reading from the device, it's memcpy()'ing between an
already read buffer and another data structure. memcpy()'ing one byte
less into an uninitialized struct doesn't make the last byte any less
undefined than copying garbage.

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

It's another thing about the length... it prevents the code from
running into an endless loop when it encounters a descriptor with
length 0.

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

I'm only touching the condition... if you don't like the message, I'd
be happy to review your patch for it.

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

[Marek Vasut]

Dear Julius Werner,

> > 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.
> 
> Weren't you the guy who was so worried about poor Chinese devices who
> don't follow the spec just two mails ago?

That would have been me, not Albert. I suspect what happens below (and will 
likely happen onwards, if we don't cut the crap soon) is a bullshit-fueled 
discussion about nothing, so just can it. It's totally unproductive and drifting 
away from the discussion about patch. ML is not the right place for duking out 
this crap ...

Please, go, break a tartelette and fix your quarrel elsewhere.

Best regards,
Marek Vasut

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

[Marek Vasut]

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

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

[Julius Werner]

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

I can change it if you insist, but I'd like to keep it to make the
code look more consistent (since later on with the interface/endpoint
descriptors, I think ignoring errors makes more sense).

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

The sizeof() thing is true for the configuration descriptor, but not
for some others (e.g. endpoint) because U-Boot reserves fields for
it's own stuff behind that. So for consistency (and safety in case of
future expansion) I went with the macros in all cases.

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

usb_get_configuration_no() now makes sure we don't read more than
USB_BUFSIZ and stores the actually read amount in wTotalLength so we
can use it without worrying here.

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

Agreed, but let's clean this up one patch at a time.

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

Jokes about Chinese crap aside, I would try to err on the side of
making it work if in any way possible, as long as the code stays safe.
This is firmware so we often can't do much error recovery... either we
can work with what we have, or we won't boot. Although I don't think
these cases will happen in practice.

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

This is the sanitization of wTotalLength I mentioned above. Maybe not
the most obvious way to do it, but it's convenient since you have to
pass the actually read length from here to usb_parse_config() somehow
(to avoid things like reading into the leftover descriptors of a
previously enumerated device).

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

[Marek Vasut]

Dear Julius Werner,

> > 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.
> 
> I can change it if you insist, but I'd like to keep it to make the
> code look more consistent (since later on with the interface/endpoint
> descriptors, I think ignoring errors makes more sense).

How would that make the code more consistent ? It seems if the device can not 
even provide valid config ep descriptor, the device is broken beyond salvation.

> > 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.
> 
> The sizeof() thing is true for the configuration descriptor, but not
> for some others (e.g. endpoint) because U-Boot reserves fields for
> it's own stuff behind that.

Urgh, then the structure defining the descriptor shall be separated out.

> So for consistency (and safety in case of
> future expansion) I went with the macros in all cases.

Dang.

> > 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.
> 
> usb_get_configuration_no() now makes sure we don't read more than
> USB_BUFSIZ and stores the actually read amount in wTotalLength so we
> can use it without worrying here.

OK

> > 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 .
> 
> Agreed, but let's clean this up one patch at a time.

Would you do a series on this maybe?

> >>                               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.
> 
> Jokes about Chinese crap aside, I would try to err on the side of
> making it work if in any way possible, as long as the code stays safe.

But obviously the descriptor is broken.

> This is firmware so we often can't do much error recovery... either we
> can work with what we have, or we won't boot. Although I don't think
> these cases will happen in practice.

So, let's just ignore broken descriptors.

> >> -     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 ?
> 
> This is the sanitization of wTotalLength I mentioned above. Maybe not
> the most obvious way to do it, but it's convenient since you have to
> pass the actually read length from here to usb_parse_config() somehow
> (to avoid things like reading into the leftover descriptors of a
> previously enumerated device).

Document this properly then.

Best regards,
Marek Vasut

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

[Julius Werner]

> How would that make the code more consistent ? It seems if the device can not
> even provide valid config ep descriptor, the device is broken beyond salvation.

Okay, sure, it's not important enough to argue about. Will resubmit it this way.

>> The sizeof() thing is true for the configuration descriptor, but not
>> for some others (e.g. endpoint) because U-Boot reserves fields for
>> it's own stuff behind that.
>
> Urgh, then the structure defining the descriptor shall be separated out.

Yes, maybe. But let's please not blow this patch up any more than it already is.

>> > 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 .
>>
>> Agreed, but let's clean this up one patch at a time.
>
> Would you do a series on this maybe?

On second thought, we already have the variable head (respectively
head->bLength) to point there... I can just use that instead.

> So, let's just ignore broken descriptors.

Done.

> Document this properly then.

I'm already adding a comment to usb_parse_config() to point that
out... I'll clarify that this includes sanitization in addition to
byte swapping.

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

[Marek Vasut]

Hi Julius,

> > How would that make the code more consistent ? It seems if the device can
> > not even provide valid config ep descriptor, the device is broken beyond
> > salvation.
> 
> Okay, sure, it's not important enough to argue about. Will resubmit it this
> way.
> 
> >> The sizeof() thing is true for the configuration descriptor, but not
> >> for some others (e.g. endpoint) because U-Boot reserves fields for
> >> it's own stuff behind that.
> > 
> > Urgh, then the structure defining the descriptor shall be separated out.
> 
> Yes, maybe. But let's please not blow this patch up any more than it
> already is.
> 
> >> > 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 .
> >> 
> >> Agreed, but let's clean this up one patch at a time.
> > 
> > Would you do a series on this maybe?
> 
> On second thought, we already have the variable head (respectively
> head->bLength) to point there... I can just use that instead.
> 
> > So, let's just ignore broken descriptors.
> 
> Done.
> 
> > Document this properly then.
> 
> I'm already adding a comment to usb_parse_config() to point that
> out... I'll clarify that this includes sanitization in addition to
> byte swapping.

THanks a lot! I'm glad you're cleaning this horror up.

Best regards,
Marek Vasut