Skip to contentSkip to conversation

usb: Use well-known descriptor sizes when parsing configuration

PATCH Superseded Julius Werner 3 participants 9 messages Jul 18, 2013 – Jul 21, 2013

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Julius Werner

author
#0 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

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 <[email protected]>
---
 common/usb.c     | 73 ++++++++++++++++++++++++++++++++++++++++++++++----------
 common/usb_hub.c | 14 ++++-------
 2 files changed, 64 insertions(+), 23 deletions(-)
common/usb.c +60 -13
index 55fff5b..ab7bafe 100644
@@ -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;
 }
common/usb_hub.c +4 -10
index 774ba63..d30962e 100644
@@ -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)),
Signature · 1 line
1.7.12.4
#1 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Hi Julius,

Quoted 9 lines · 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 <[email protected]>

---

Please do not forget patch history.

Quoted 17 lines
 common/usb.c     | 73 ++++++++++++++++++++++++++++++++++++++++++++++----------
 common/usb_hub.c | 14 ++++-------
 2 files changed, 64 insertions(+), 23 deletions(-)
common/usb.c +1 -0
index 55fff5b..ab7bafe 100644
@@ -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)

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 368

Quoted 9 lines
common/usb.c +3 -2
@@ -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).

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 374

Quoted 6 lines
common/usb.c +1 -1
 	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.

Quoted 14 lines
common/usb.c +8 -0
 		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 */

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 397

Quoted 10 lines
common/usb.c +7 -1
+					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.

Quoted 18 lines
common/usb.c +9 -0
 				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];

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 431

Quoted 10 lines
common/usb.c +8 -0
+			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.

Quoted 13 lines
common/usb.c +4 -1
 			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");

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 458

Quoted 10 lines
common/usb.c +9 -0
+				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.

Quoted 60 lines
common/usb.c +10 -8
 			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;
 }
common/usb_hub.c +1 -4
index 774ba63..d30962e 100644
@@ -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) {

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 348

Quoted 10 lines
common/usb_hub.c +2 -5
-		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.

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 351

Quoted 4 lines
common/usb_hub.c +1 -1
 		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.

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 354

Quoted 3 lines
common/usb_hub.c +0 -0
 	/* adjust 16bit values */
 	put_unaligned(le16_to_cpu(get_unaligned(
 			&descriptor->wHubCharacteristics)),

Amicalement,

Signature · 1 line
Albert.
author
#2 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Quoted 5 lines

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.

Quoted 5 lines

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

Quoted 4 lines

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.

Quoted 7 lines

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 348

Quoted 4 lines
common/usb_hub.c +1 -1
-     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.

#3 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Dear Julius Werner,

Quoted 9 lines
Quoted 6 lines

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

#4 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

In reply to Julius Werner

Dear Julius Werner,

Quoted 9 lines

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 <[email protected]>

---

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

Quoted 19 lines
 common/usb.c     | 73
++++++++++++++++++++++++++++++++++++++++++++++---------- common/usb_hub.c
| 14 ++++-------
 2 files changed, 64 insertions(+), 23 deletions(-)
common/usb.c +0 -0
index 55fff5b..ab7bafe 100644
@@ -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,

common/usb.c +0 -0
 			unsigned char *buffer, int cfgno)

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 366

Quoted 7 lines
common/usb.c +2 -2
@@ -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.

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 368

Quoted 2 lines
common/usb.c +1 -0
+	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 :(

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 374

Quoted 6 lines
common/usb.c +1 -1
 	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.

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 381

Quoted 7 lines
common/usb.c +5 -0
 		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.

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 383

Quoted 2 lines
common/usb.c +0 -0
 			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-

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 390

Quoted 9 lines

bInterfaceNumber and curr_if_num .

common/usb.c +5 -0
 				/* 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

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 395

Quoted 6 lines
common/usb.c +3 -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]);

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

Quoted 8 lines
common/usb.c +2 -0
+				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;

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 419

Quoted 10 lines
common/usb.c +9 -0
 		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.

Quoted 5 lines
common/usb.c +3 -0
 			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);

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 433

Quoted 10 lines
common/usb.c +6 -1
+				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() ?

Quoted 46 lines
common/usb.c +16 -6
 			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) {

common/usb.c int usb_get_configuration_no(struct usb_device *dev, · line 563

Quoted 10 lines
common/usb.c +5 -2
+		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 ?

Quoted 31 lines
common/usb.c +0 -0
 	return result;
 }
common/usb_hub.c +1 -1
index 774ba63..d30962e 100644
@@ -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;
common/usb_hub.c +1 -8
 	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) {

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 354

Quoted 10 lines
common/usb_hub.c +2 -1
+	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

author
#5 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Quoted 2 lines

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

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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-

Quoted 1 line

bInterfaceNumber and curr_if_num .

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

Quoted 9 lines

common/usb.c static int usb_parse_config(struct usb_device *dev, · line 395

Quoted 6 lines
                              if_desc = &dev->config.if_desc[ifno];
                              dev->config.no_of_if++;
common/usb.c +3 -1
-                             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.

Quoted 7 lines

common/usb.c int usb_get_configuration_no(struct usb_device *dev, · line 563

Quoted 5 lines
common/usb.c +3 -2
-     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).

#6 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Dear Julius Werner,

Quoted 7 lines
Quoted 3 lines

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.

Quoted 10 lines
Quoted 6 lines

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.

Quoted 2 lines

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

Dang.

Quoted 7 lines
Quoted 3 lines

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

Quoted 6 lines
Quoted 4 lines

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-

Quoted 1 line

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?

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
                              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]);
Quoted 4 lines
Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

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

Quoted 7 lines
Quoted 1 line

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

author
#7 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Quoted 2 lines

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.

Quoted 5 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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-

Quoted 1 line

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.

Quoted 1 line

So, let's just ignore broken descriptors.

Done.

Quoted 1 line

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.

#8 lore raw

PATCH v2 1/1 usb: Use well-known descriptor sizes when parsing configuration

Hi Julius,

Quoted 31 lines
Quoted 3 lines

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.

Quoted 5 lines
Quoted 3 lines

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.

Quoted 9 lines
Quoted 7 lines
Quoted 5 lines

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-

Quoted 1 line

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.

Quoted 1 line

So, let's just ignore broken descriptors.

Quoted 7 lines

Done.

Quoted 1 line

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