[PATCH 0/6] usb: check hub port and descriptor counts during enumeration

[PATCH 0/6] usb: check hub port and descriptor counts during enumeration

[Ana Bobillo]

U-Boot takes several counts from the descriptors of a USB device and
uses them to index fixed-size arrays without checking them. A device, or
a hub, whose counts claim more than it describes, or more than U-Boot
has room for, makes enumeration and the class drivers read and write
past those arrays.

This series checks each of them where it is parsed:

1. USB_MAXCHILDREN goes from 8 to 31, as in Linux, as xHCI root hubs
   often have more than 8 ports
2. usb_hub_configure() uses at most USB_MAXCHILDREN ports of a hub
3. usb tree, without driver model, no longer reads one past the ports
4. usb_parse_config() lowers bNumInterfaces and each bNumEndpoints to
   what it stored, and keeps what fits of a device with more interfaces
   or endpoints than there is room for
5. usb_parse_config() skips endpoint descriptors for endpoint zero,
   which xhci_get_ep_index() turns into the endpoint index -1
6. usb_select_config() drops a device whose configuration can't be
   parsed

The tests for patches 2 and 4 to 6 use the sandbox USB emulators. With
sandbox_defconfig and CONFIG_ASAN, the five new tests fail on main and
pass with the series applied, each patch builds and passes the tests
present at that point, and the whole ut dm suite still passes.

CI: Azure build 13714 passed, all jobs, with this series on
https://github.com/u-boot/u-boot/pull/1064, a CI-only pull request,
merged with main 3973f3c4d24:
https://dev.azure.com/u-boot/u-boot/_build/results?buildId=13714

Ana Bobillo (6):
  usb: raise USB_MAXCHILDREN to 31
  usb: hub: don't use more ports than USB_MAXCHILDREN
  cmd: usb: don't look past the last port in usb tree
  usb: keep interface and endpoint counts within what was parsed
  usb: skip endpoint descriptors for endpoint zero
  usb: don't use a device whose configuration can't be parsed

 cmd/usb.c                      |   2 +-
 common/usb.c                   |  38 ++++-
 common/usb_hub.c               |  21 ++-
 drivers/usb/emul/sandbox_hub.c |  40 ++++-
 include/usb.h                  |   2 +-
 test/dm/usb.c                  | 262 +++++++++++++++++++++++++++++++++
 6 files changed, 350 insertions(+), 15 deletions(-)


base-commit: 3973f3c4d24df75ca90d7c0a9f03e5e5c9b48f32

[PATCH 1/6] usb: raise USB_MAXCHILDREN to 31

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

A hub's per-port state is sized by USB_MAXCHILDREN: the DeviceRemovable
and PortPowerCtrlMask bitmaps and overcurrent_count[] in struct
usb_hub_device, and children[] in struct usb_device without driver
model. At 8 this is below the port count of many root hubs. An xHCI root
hub reports every USB 2 and USB 3 port of its controller (HCS_MAX_PORTS),
more than 8 on many PC chipsets, and usb_scan_port() then counts
over-current events past the end of overcurrent_count[] for the higher
ports.

Use 31, as Linux does: the port bitmaps, with their bit for the hub
itself, then fill four bytes. A following patch keeps
usb_hub_configure() from using more ports than this.

This grows struct usb_hub_device by 96 bytes and, without driver model,
children[] by 23 pointers: about 4.4 KiB more BSS on a 32-bit board
without driver model (USB_MAX_DEVICE devices, USB_MAX_HUB hubs).

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 include/usb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/usb.h b/include/usb.h
index dab23753f0c..76a3efa49d3 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -38,7 +38,7 @@ extern bool usb_started; /* flag for the started/stopped USB status */
 #define USB_MAXCONFIG			8
 #define USB_MAXINTERFACES		8
 #define USB_MAXENDPOINTS		16
-#define USB_MAXCHILDREN			8	/* This is arbitrary */
+#define USB_MAXCHILDREN			31	/* As Linux: the port bitmaps fit 4 bytes */
 #define USB_MAX_HUB			16
 
 #define USB_CNTL_TIMEOUT 100 /* 100ms timeout */

[PATCH 2/6] usb: hub: don't use more ports than USB_MAXCHILDREN

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

usb_hub_configure() takes the port count from the hub descriptor's
bNbrPorts, which a hub may set to anything up to 255. The port bitmaps
it then copies (DeviceRemovable, PortPowerCtrlMask), overcurrent_count[]
and, without driver model, children[] only have room for USB_MAXCHILDREN
ports. On a hub that claims more, the bitmap copies run past their
arrays and the port scan indexes past the other two.

Only use the first USB_MAXCHILDREN ports of such a hub. Linux refuses
the hub instead, but its xHCI driver first limits the root hub to the
ports the hub code supports, while ours passes the controller's port
count through. Limiting the ports keeps such a root hub working.

Make the sandbox hub emulator refuse requests for ports it doesn't have,
rather than index past its port state, and add a test with a hub that
reports one port more than USB_MAXCHILDREN.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 common/usb_hub.c               | 21 ++++++++++++----
 drivers/usb/emul/sandbox_hub.c | 29 +++++++++++++++++++---
 test/dm/usb.c                  | 44 ++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 6a4bcec00bc..6bfe3784a97 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -658,6 +658,20 @@ static int usb_hub_configure(struct usb_device *dev)
 	put_unaligned(le16_to_cpu(get_unaligned(
 			&descriptor->wHubCharacteristics)),
 			&hub->desc.wHubCharacteristics);
+
+	/*
+	 * The port bitmaps below and overcurrent_count[] (and children[]
+	 * without driver model) hold USB_MAXCHILDREN ports, so don't use
+	 * more than that.
+	 */
+	dev->maxchild = descriptor->bNbrPorts;
+	if (dev->maxchild > USB_MAXCHILDREN) {
+		printf("Hub has %d ports, only using %d\n", dev->maxchild,
+		       USB_MAXCHILDREN);
+		dev->maxchild = USB_MAXCHILDREN;
+	}
+	debug("%d ports detected\n", dev->maxchild);
+
 	/* set the bitmap */
 	bitmap = (unsigned char *)&hub->desc.u.hs.DeviceRemovable[0];
 	/* devices not removable by default */
@@ -665,17 +679,14 @@ static int usb_hub_configure(struct usb_device *dev)
 	bitmap = (unsigned char *)&hub->desc.u.hs.PortPowerCtrlMask[0];
 	memset(bitmap, 0xff, (USB_MAXCHILDREN+1+7)/8); /* PowerMask = 1B */
 
-	for (i = 0; i < ((hub->desc.bNbrPorts + 1 + 7)/8); i++)
+	for (i = 0; i < ((dev->maxchild + 1 + 7) / 8); i++)
 		hub->desc.u.hs.DeviceRemovable[i] =
 			descriptor->u.hs.DeviceRemovable[i];
 
-	for (i = 0; i < ((hub->desc.bNbrPorts + 1 + 7)/8); i++)
+	for (i = 0; i < ((dev->maxchild + 1 + 7) / 8); i++)
 		hub->desc.u.hs.PortPowerCtrlMask[i] =
 			descriptor->u.hs.PortPowerCtrlMask[i];
 
-	dev->maxchild = descriptor->bNbrPorts;
-	debug("%d ports detected\n", dev->maxchild);
-
 	hubCharacteristics = get_unaligned(&hub->desc.wHubCharacteristics);
 	switch (hubCharacteristics & HUB_CHAR_LPSM) {
 	case 0x00:
diff --git a/drivers/usb/emul/sandbox_hub.c b/drivers/usb/emul/sandbox_hub.c
index 3b3e59f978f..066898f005e 100644
--- a/drivers/usb/emul/sandbox_hub.c
+++ b/drivers/usb/emul/sandbox_hub.c
@@ -198,6 +198,23 @@ static int clrset_post_state(struct udevice *hub, int port, int clear, int set)
 	return ret;
 }
 
+/**
+ * hub_port() - Get the port that a port request is for
+ *
+ * @setup:	Request
+ * Return: port number (numbered from 0), or -EINVAL if the hub doesn't have
+ * the port
+ */
+static int hub_port(struct devrequest *setup)
+{
+	int port = (setup->index & 0xff) - 1;
+
+	if (port < 0 || port >= SANDBOX_NUM_PORTS)
+		return -EINVAL;
+
+	return port;
+}
+
 static int sandbox_hub_submit_control_msg(struct udevice *bus,
 					  struct usb_device *udev,
 					  unsigned long pipe,
@@ -228,7 +245,9 @@ static int sandbox_hub_submit_control_msg(struct udevice *bus,
 				struct usb_port_status *portsts = buffer;
 				int port;
 
-				port = (setup->index & USB_HUB_PORT_MASK) - 1;
+				port = hub_port(setup);
+				if (port < 0)
+					return port;
 				portsts->wPortStatus = priv->status[port];
 				portsts->wPortChange = priv->change[port];
 				udev->status = 0;
@@ -247,7 +266,9 @@ static int sandbox_hub_submit_control_msg(struct udevice *bus,
 			case USB_REQ_SET_FEATURE: {
 				int port;
 
-				port = (setup->index & USB_HUB_PORT_MASK) - 1;
+				port = hub_port(setup);
+				if (port < 0)
+					return port;
 				debug("set feature port=%x, feature=%x\n",
 				      port, setup->value);
 				if (setup->value < USB_PORT_FEAT_C_CONNECTION) {
@@ -261,7 +282,9 @@ static int sandbox_hub_submit_control_msg(struct udevice *bus,
 			case USB_REQ_CLEAR_FEATURE: {
 				int port;
 
-				port = (setup->index & USB_HUB_PORT_MASK) - 1;
+				port = hub_port(setup);
+				if (port < 0)
+					return port;
 				debug("clear feature port=%x, feature=%x\n",
 				      port, setup->value);
 				if (setup->value < USB_PORT_FEAT_C_CONNECTION) {
diff --git a/test/dm/usb.c b/test/dm/usb.c
index fa894c1096e..7f803f9fe01 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -456,3 +456,47 @@ static int dm_test_usb_keyb(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_usb_keyb, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/* test that a hub reporting more ports than we can track is limited */
+static int dm_test_usb_hub_ports(struct unit_test_state *uts)
+{
+	static struct usb_hub_descriptor hub_desc;
+	static void *desc_list[8];
+	struct usb_generic_descriptor **ptr;
+	struct usb_dev_plat *plat;
+	struct udevice *emul, *hub, *dev;
+	struct usb_device *udev;
+	int i;
+
+	/* Have the hub emulator report more ports than USB_MAXCHILDREN */
+	ut_assertok(uclass_find_device_by_name(UCLASS_USB_EMUL, "hub-emul",
+					       &emul));
+	plat = dev_get_parent_plat(emul);
+	for (i = 0, ptr = plat->desc_list; *ptr; i++, ptr++) {
+		ut_assert(i < ARRAY_SIZE(desc_list) - 1);
+		desc_list[i] = *ptr;
+		if ((*ptr)->bDescriptorType == USB_DT_HUB) {
+			memcpy(&hub_desc, *ptr, sizeof(hub_desc));
+			hub_desc.bNbrPorts = USB_MAXCHILDREN + 1;
+			desc_list[i] = &hub_desc;
+		}
+	}
+	desc_list[i] = NULL;
+	plat->desc_list = (struct usb_generic_descriptor **)desc_list;
+
+	state_set_skip_delays(true);
+	ut_assertok(usb_init());
+	ut_assertok(uclass_find_device_by_name(UCLASS_USB_HUB, "hub", &hub));
+	udev = dev_get_parent_priv(hub);
+	ut_assertnonnull(udev);
+	ut_asserteq(USB_MAXCHILDREN, udev->maxchild);
+
+	/* The devices on its first ports are still there */
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 1, &dev));
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 2, &dev));
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_hub_ports, UTF_SCAN_PDATA | UTF_SCAN_FDT);

[PATCH 3/6] cmd: usb: don't look past the last port in usb tree

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Without driver model, usb_show_tree_graph() looks for a later sibling
of a device in its parent's children[], from the port after the
device's own. The loop tests its index before incrementing it, so its
last pass reads children[maxchild], one past the ports of the hub, and
past the end of children[] for a hub with USB_MAXCHILDREN ports.

Increment the index before testing it.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 cmd/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/usb.c b/cmd/usb.c
index 13a2996c1f0..278ecac7390 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -372,7 +372,7 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
 				/* found our pointer, see if we have a
 				 * little sister
 				 */
-				while (i++ < dev->parent->maxchild) {
+				while (++i < dev->parent->maxchild) {
 					if (dev->parent->children[i] != NULL) {
 						/* found a sister */
 						last_child = 0;

[PATCH 4/6] usb: keep interface and endpoint counts within what was parsed

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

usb_parse_config() stores at most USB_MAXINTERFACES interfaces, with
the endpoint descriptors it finds for each, but leaves bNumInterfaces
and each interface's bNumEndpoints as the device sent them. These counts
are what usb_set_maxpacket(), usb_set_interface() and the class drivers
(usb_storage, usb_kbd, usb_ether) walk if_desc[] and ep_desc[] by. A
device whose counts claim more than it describes, or more than there is
room for, makes them read past what was stored, and past the end of
those arrays.

Lower the counts to what was stored once the configuration is parsed.
For a device with more interfaces or endpoints than there is room for,
keep the ones that fit, as the comment there already intends, rather
than return an error that nothing checks: stop at the first interface
too many, and skip each endpoint too many. Don't take the companion
descriptor of a skipped endpoint for that of the endpoint before it.

Add tests for both cases.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 common/usb.c  |  24 ++++++--
 test/dm/usb.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+), 4 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 6a4ad346f4b..05502da6b30 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -385,6 +385,7 @@ static int usb_parse_config(struct usb_device *dev,
 	int index, ifno, epno, curr_if_num;
 	u16 ep_wMaxPacketSize;
 	struct usb_interface *if_desc = NULL;
+	int i;
 
 	ifno = -1;
 	epno = -1;
@@ -428,7 +429,7 @@ static int usb_parse_config(struct usb_device *dev,
 				if (ifno >= USB_MAXINTERFACES) {
 					puts("Too many USB interfaces!\n");
 					/* try to go on with what we have */
-					return -EINVAL;
+					goto out;
 				}
 				if_desc = &dev->config.if_desc[ifno];
 				dev->config.no_of_if++;
@@ -447,6 +448,8 @@ static int usb_parse_config(struct usb_device *dev,
 			}
 			break;
 		case USB_DT_ENDPOINT:
+			/* no companion descriptor goes with an endpoint we skip */
+			epno = -1;
 			if (head->bLength != USB_DT_ENDPOINT_SIZE &&
 			    head->bLength != USB_DT_ENDPOINT_AUDIO_SIZE) {
 				printf("ERROR: Invalid USB EP length (%d)\n",
@@ -462,14 +465,14 @@ static int usb_parse_config(struct usb_device *dev,
 				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) {
+			if (if_desc->no_of_ep >= USB_MAXENDPOINTS) {
 				printf("Interface %d has too many endpoints!\n",
 					if_desc->desc.bInterfaceNumber);
-				return -EINVAL;
+				break;
 			}
 			/* found an endpoint */
+			epno = if_desc->no_of_ep;
 			if_desc->no_of_ep++;
 			memcpy(&if_desc->ep_desc[epno], head,
 				USB_DT_ENDPOINT_SIZE);
@@ -525,6 +528,19 @@ static int usb_parse_config(struct usb_device *dev,
 		index += head->bLength;
 		head = (struct usb_descriptor_header *)&buffer[index];
 	}
+out:
+	/*
+	 * Interfaces and endpoints are walked by the counts in their
+	 * descriptors later on, so don't let these claim more than we stored.
+	 */
+	if (dev->config.desc.bNumInterfaces > dev->config.no_of_if)
+		dev->config.desc.bNumInterfaces = dev->config.no_of_if;
+	for (i = 0; i < dev->config.no_of_if; i++) {
+		if_desc = &dev->config.if_desc[i];
+		if (if_desc->desc.bNumEndpoints > if_desc->no_of_ep)
+			if_desc->desc.bNumEndpoints = if_desc->no_of_ep;
+	}
+
 	return 0;
 }
 
diff --git a/test/dm/usb.c b/test/dm/usb.c
index 7f803f9fe01..145ce79dfec 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -500,3 +500,157 @@ static int dm_test_usb_hub_ports(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_usb_hub_ports, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/*
+ * Descriptors of a device with a vendor-specific interface, which is bound
+ * to the generic driver whatever its endpoints. The tests below give them,
+ * or copies with errors in them, to the emulator on port 1 in place of its
+ * own, to check how a configuration that a device gets wrong is parsed.
+ */
+static struct usb_device_descriptor vendor_device_desc = {
+	.bLength		= sizeof(vendor_device_desc),
+	.bDescriptorType	= USB_DT_DEVICE,
+	.bcdUSB			= cpu_to_le16(0x0200),
+	.idVendor		= cpu_to_le16(0x1234),
+	.idProduct		= cpu_to_le16(0x5678),
+	.bMaxPacketSize0	= 64,
+	.bNumConfigurations	= 1,
+};
+
+static struct usb_config_descriptor vendor_config0 = {
+	.bLength		= sizeof(vendor_config0),
+	.bDescriptorType	= USB_DT_CONFIG,
+	/* wTotalLength is set up by usb_emul_setup_device() */
+	.bNumInterfaces		= 1,
+	.bmAttributes		= 1 << 7,
+	.bMaxPower		= 50,
+};
+
+static struct usb_interface_descriptor vendor_interface0 = {
+	.bLength		= sizeof(vendor_interface0),
+	.bDescriptorType	= USB_DT_INTERFACE,
+	.bNumEndpoints		= 1,
+	.bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
+};
+
+static struct usb_endpoint_descriptor vendor_endpoint1_in = {
+	.bLength		= USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType	= USB_DT_ENDPOINT,
+	.bEndpointAddress	= 1 | USB_DIR_IN,
+	.bmAttributes		= USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize		= cpu_to_le16(512),
+};
+
+/**
+ * scan_port1() - Scan the bus with a device on port 1 that has the given
+ * descriptors
+ *
+ * @uts:	Test state
+ * @desc_list:	Descriptors of the device, terminated by NULL
+ * @udevp:	Returns the device found on port 1, or NULL if none was set up
+ * Return: 0 if OK, -ve on error
+ */
+static int scan_port1(struct unit_test_state *uts, void **desc_list,
+		      struct usb_device **udevp)
+{
+	struct udevice *emul, *hub, *dev;
+
+	ut_assertok(uclass_find_device_by_name(UCLASS_USB_EMUL,
+					       "flash-stick@0", &emul));
+	ut_assertok(usb_emul_setup_device(emul, NULL, desc_list));
+
+	state_set_skip_delays(true);
+	ut_assertok(usb_init());
+
+	*udevp = NULL;
+	ut_assertok(uclass_find_device_by_name(UCLASS_USB_HUB, "hub", &hub));
+	device_foreach_child(dev, hub) {
+		struct usb_device *udev = dev_get_parent_priv(dev);
+
+		if (device_active(dev) && udev->portnr == 1)
+			*udevp = udev;
+	}
+
+	return 0;
+}
+
+/* test that interface and endpoint counts are limited to those described */
+static int dm_test_usb_config_counts(struct unit_test_state *uts)
+{
+	static struct usb_config_descriptor config0;
+	static struct usb_interface_descriptor interface0;
+	static void *desc_list[] = {
+		&vendor_device_desc,
+		&config0,
+		&interface0,
+		&vendor_endpoint1_in,
+		NULL,
+	};
+	struct usb_device *udev;
+
+	config0 = vendor_config0;
+	config0.bNumInterfaces = USB_MAXINTERFACES + 1;
+	interface0 = vendor_interface0;
+	interface0.bNumEndpoints = USB_MAXENDPOINTS + 1;
+	ut_assertok(scan_port1(uts, desc_list, &udev));
+	ut_assertnonnull(udev);
+	ut_asserteq(1, udev->config.no_of_if);
+	ut_asserteq(1, udev->config.desc.bNumInterfaces);
+	ut_asserteq(1, udev->config.if_desc[0].no_of_ep);
+	ut_asserteq(1, udev->config.if_desc[0].desc.bNumEndpoints);
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_config_counts, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/* test that a device with more interfaces and endpoints than we keep works */
+static int dm_test_usb_config_too_many(struct unit_test_state *uts)
+{
+	static struct usb_config_descriptor config0;
+	static struct usb_interface_descriptor interface[USB_MAXINTERFACES + 1];
+	static struct usb_endpoint_descriptor endpoint[USB_MAXENDPOINTS + 1];
+	static void *desc_list[3 + ARRAY_SIZE(interface) + ARRAY_SIZE(endpoint)];
+	struct usb_device *udev;
+	int i, n = 0;
+
+	config0 = vendor_config0;
+	config0.bNumInterfaces = ARRAY_SIZE(interface);
+	for (i = 0; i < ARRAY_SIZE(interface); i++) {
+		interface[i] = vendor_interface0;
+		interface[i].bInterfaceNumber = i;
+		interface[i].bNumEndpoints = 0;
+	}
+	desc_list[n++] = &vendor_device_desc;
+	desc_list[n++] = &config0;
+
+	/* The first interface has one endpoint too many */
+	interface[0].bNumEndpoints = ARRAY_SIZE(endpoint);
+	desc_list[n++] = &interface[0];
+	for (i = 0; i < ARRAY_SIZE(endpoint); i++) {
+		endpoint[i] = vendor_endpoint1_in;
+		endpoint[i].bEndpointAddress = 1 + i % 15;
+		if (i < 15)
+			endpoint[i].bEndpointAddress |= USB_DIR_IN;
+		desc_list[n++] = &endpoint[i];
+	}
+
+	/* and there is one interface too many */
+	for (i = 1; i < ARRAY_SIZE(interface); i++)
+		desc_list[n++] = &interface[i];
+	desc_list[n] = NULL;
+
+	ut_assertok(scan_port1(uts, desc_list, &udev));
+	ut_assertnonnull(udev);
+	ut_asserteq(USB_MAXINTERFACES, udev->config.no_of_if);
+	ut_asserteq(USB_MAXINTERFACES, udev->config.desc.bNumInterfaces);
+	for (i = 0; i < USB_MAXINTERFACES; i++)
+		ut_asserteq(i, udev->config.if_desc[i].desc.bInterfaceNumber);
+	ut_asserteq(USB_MAXENDPOINTS, udev->config.if_desc[0].no_of_ep);
+	ut_asserteq(USB_MAXENDPOINTS,
+		    udev->config.if_desc[0].desc.bNumEndpoints);
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_config_too_many, UTF_SCAN_PDATA | UTF_SCAN_FDT);

[PATCH 5/6] usb: skip endpoint descriptors for endpoint zero

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Endpoint zero is the default control endpoint, which every device has
and no interface describes. usb_parse_config() still stores an endpoint
descriptor numbered zero like any other, so it reaches the users of
ep_desc[]: usb_set_maxpacket() overwrites endpoint zero's packet sizes
with it, and on xHCI, xhci_get_ep_index() gives an OUT endpoint zero
the index -1, which xhci_init_ep_contexts_if() then uses for the
device's endpoint contexts and rings.

Skip such a descriptor, and the companion descriptor after it, as Linux
does in usb_parse_endpoint(). Add a test with a device that has one,
which also checks that endpoint zero keeps its packet size.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 common/usb.c  |  7 +++++++
 test/dm/usb.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index 05502da6b30..619132512dd 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -466,6 +466,13 @@ static int usb_parse_config(struct usb_device *dev,
 				break;
 			}
 			if_desc = &dev->config.if_desc[ifno];
+			/* endpoint zero is the default control endpoint */
+			if (usb_endpoint_num((struct usb_endpoint_descriptor *)
+					     head) == 0) {
+				printf("Interface %d has a descriptor for endpoint zero, skipping\n",
+				       if_desc->desc.bInterfaceNumber);
+				break;
+			}
 			if (if_desc->no_of_ep >= USB_MAXENDPOINTS) {
 				printf("Interface %d has too many endpoints!\n",
 					if_desc->desc.bInterfaceNumber);
diff --git a/test/dm/usb.c b/test/dm/usb.c
index 145ce79dfec..4975a952d87 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -574,6 +574,42 @@ static int scan_port1(struct unit_test_state *uts, void **desc_list,
 	return 0;
 }
 
+/* test that a descriptor for endpoint zero is ignored */
+static int dm_test_usb_config_ep0(struct unit_test_state *uts)
+{
+	static struct usb_interface_descriptor interface0;
+	static struct usb_endpoint_descriptor endpoint0 = {
+		.bLength		= USB_DT_ENDPOINT_SIZE,
+		.bDescriptorType	= USB_DT_ENDPOINT,
+		.bEndpointAddress	= 0,
+		.bmAttributes		= USB_ENDPOINT_XFER_BULK,
+		.wMaxPacketSize		= cpu_to_le16(512),
+	};
+	static void *desc_list[] = {
+		&vendor_device_desc,
+		&vendor_config0,
+		&interface0,
+		&endpoint0,
+		&vendor_endpoint1_in,
+		NULL,
+	};
+	struct usb_device *udev;
+
+	interface0 = vendor_interface0;
+	interface0.bNumEndpoints = 2;
+	ut_assertok(scan_port1(uts, desc_list, &udev));
+	ut_assertnonnull(udev);
+	ut_asserteq(1, udev->config.if_desc[0].no_of_ep);
+	ut_asserteq(1 | USB_DIR_IN,
+		    udev->config.if_desc[0].ep_desc[0].bEndpointAddress);
+	ut_asserteq(64, udev->epmaxpacketin[0]);
+	ut_asserteq(64, udev->epmaxpacketout[0]);
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_config_ep0, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
 /* test that interface and endpoint counts are limited to those described */
 static int dm_test_usb_config_counts(struct unit_test_state *uts)
 {

[PATCH 6/6] usb: don't use a device whose configuration can't be parsed

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

usb_select_config() ignores what usb_parse_config() returns, so a
device whose configuration doesn't start with a valid configuration
descriptor is set up anyway, with an empty configuration. Since the
previous patches, this is the only case where usb_parse_config() fails:
a device with more interfaces or endpoints than there is room for is
parsed as far as there is room.

Skip such a device, as is done when its configuration can't be read.

The hub then disables the device's port, and the next device found
gets the same address. Have the sandbox hub emulator drop the address
of a device on a disabled port, as such a device no longer answers, so
that the emulator of the next device is the one found at that address.
Add a test with a configuration descriptor of the wrong length, which
also checks that the devices on the other ports are still found.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 common/usb.c                   |  7 ++++++-
 drivers/usb/emul/sandbox_hub.c | 11 +++++++++++
 test/dm/usb.c                  | 28 ++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/common/usb.c b/common/usb.c
index 619132512dd..87f0590bbd5 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1232,8 +1232,13 @@ int usb_select_config(struct usb_device *dev)
 		free(tmpbuf);
 		return err;
 	}
-	usb_parse_config(dev, tmpbuf, 0);
+	err = usb_parse_config(dev, tmpbuf, 0);
 	free(tmpbuf);
+	if (err) {
+		printf("usb_new_device: Cannot parse configuration, skipping device %04x:%04x\n",
+		       dev->descriptor.idVendor, dev->descriptor.idProduct);
+		return err;
+	}
 	usb_set_maxpacket(dev);
 	/*
 	 * we set the default configuration here
diff --git a/drivers/usb/emul/sandbox_hub.c b/drivers/usb/emul/sandbox_hub.c
index 066898f005e..c5a5470f9fd 100644
--- a/drivers/usb/emul/sandbox_hub.c
+++ b/drivers/usb/emul/sandbox_hub.c
@@ -190,6 +190,17 @@ static int clrset_post_state(struct udevice *hub, int port, int clear, int set)
 			}
 		}
 	}
+	if (clear & USB_PORT_STAT_ENABLE) {
+		enum usb_device_speed speed;
+		struct udevice *dev = hub_find_device(hub, port, &speed);
+
+		/* A device on a disabled port no longer answers at its address */
+		if (dev) {
+			struct usb_dev_plat *plat = dev_get_parent_plat(dev);
+
+			plat->devnum = 0;
+		}
+	}
 	*change |= *status & clear;
 	*change |= ~*status & set;
 	*change &= 0x1f;
diff --git a/test/dm/usb.c b/test/dm/usb.c
index 4975a952d87..80763eb8e7b 100644
--- a/test/dm/usb.c
+++ b/test/dm/usb.c
@@ -690,3 +690,31 @@ static int dm_test_usb_config_too_many(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_usb_config_too_many, UTF_SCAN_PDATA | UTF_SCAN_FDT);
+
+/* test that a device whose configuration cannot be parsed is not used */
+static int dm_test_usb_config_bad(struct unit_test_state *uts)
+{
+	static struct usb_config_descriptor config0;
+	static void *desc_list[] = {
+		&vendor_device_desc,
+		&config0,
+		&vendor_interface0,
+		&vendor_endpoint1_in,
+		NULL,
+	};
+	struct usb_device *udev;
+	struct udevice *dev;
+
+	config0 = vendor_config0;
+	config0.bLength = USB_DT_CONFIG_SIZE - 1;
+	ut_assertok(scan_port1(uts, desc_list, &udev));
+	ut_assertnull(udev);
+
+	/* The devices on the other ports are still there */
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
+	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 1, &dev));
+	ut_assertok(usb_stop());
+
+	return 0;
+}
+DM_TEST(dm_test_usb_config_bad, UTF_SCAN_PDATA | UTF_SCAN_FDT);