diff mbox

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

Message ID 1373675416-21559-1-git-send-email-jwerner@chromium.org
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Julius Werner July 13, 2013, 12:30 a.m. UTC 
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(-)

Comments

Albert ARIBAUD July 13, 2013, 7:23 a.m. UTC | #1 
Hi Julius,

On Fri, 12 Jul 2013 17:30:16 -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.

Question from the offspring of an USB ignoramus and a Devil's advocate:
if the device, rather than relying on well-known sizes,  provides its
own values for lengths, maybe it's because it will let you read or write
only that much, and if you try to read more, you might put the device
in an unclean, or worse, non-functional, state?

In that case, trying to read a well-known length instead could cause
more issues than reading the device-maker-specified length, could it
not?

BTW, what is the real-world issue (hardware, conditions, expected
outcome, actual outcome) which made this change needed?

Amicalement,
diff mbox

Patch

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