[PATCH] USB: Accept devices with funky interface/altsetting numbers

Now that all the USB drivers have been audited, we can safely accept
devices that have noncompliant numbering for their interfaces or
altsettings.  This patch skips bad or duplicate descriptors, allows gaps
in the numbering, accepts more or fewer interfaces than bNumInterfaces,
and logs warnings describing all these things.  Also, the debugging log
messages have been improved by David Brownell.

This should please a sizeable group of users.

drivers/usb/core/config.c

@@ -18,6 +18,11 @@
 #define USB_MAXCONFIG			8	/* Arbitrary limit */
 
 
+static inline const char *plural(int n)
+{
+	return (n == 1 ? "" : "s");
+}
+
 static int find_next_descriptor(unsigned char *buffer, int size,
     int dt1, int dt2, int *num_skipped)
 {
@@ -26,7 +31,7 @@ static int find_next_descriptor(unsigned char *buffer, int size,
 	unsigned char *buffer0 = buffer;
 
 	/* Find the next descriptor of type dt1 or dt2 */
-	while (size >= sizeof(struct usb_descriptor_header)) {
+	while (size > 0) {
 		h = (struct usb_descriptor_header *) buffer;
 		if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
 			break;
@@ -43,46 +48,46 @@ static int find_next_descriptor(unsigned char *buffer, int size,
 }
 
 static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
-    int asnum, struct usb_host_endpoint *endpoint,
+    int asnum, struct usb_host_interface *ifp, int num_ep,
     unsigned char *buffer, int size)
 {
 	unsigned char *buffer0 = buffer;
-	struct usb_descriptor_header *header;
+	struct usb_endpoint_descriptor *d;
+	struct usb_host_endpoint *endpoint;
 	int n, i;
 
-	header = (struct usb_descriptor_header *)buffer;
-	if (header->bDescriptorType != USB_DT_ENDPOINT) {
-		dev_err(ddev, "config %d interface %d altsetting %d has an "
-		    "unexpected descriptor of type 0x%X, "
-		    "expecting endpoint type 0x%X\n",
-		    cfgno, inum, asnum,
-		    header->bDescriptorType, USB_DT_ENDPOINT);
-		return -EINVAL;
-	}
+	d = (struct usb_endpoint_descriptor *) buffer;
+	buffer += d->bLength;
+	size -= d->bLength;
 
-	if (header->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE)
-		memcpy(&endpoint->desc, buffer, USB_DT_ENDPOINT_AUDIO_SIZE);
-	else if (header->bLength >= USB_DT_ENDPOINT_SIZE)
-		memcpy(&endpoint->desc, buffer, USB_DT_ENDPOINT_SIZE);
+	if (d->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE)
+		n = USB_DT_ENDPOINT_AUDIO_SIZE;
+	else if (d->bLength >= USB_DT_ENDPOINT_SIZE)
+		n = USB_DT_ENDPOINT_SIZE;
 	else {
-		dev_err(ddev, "config %d interface %d altsetting %d has an "
-		    "invalid endpoint descriptor of length %d\n",
-		    cfgno, inum, asnum, header->bLength);
-		return -EINVAL;
+		dev_warn(ddev, "config %d interface %d altsetting %d has an "
+		    "invalid endpoint descriptor of length %d, skipping\n",
+		    cfgno, inum, asnum, d->bLength);
+		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
-	i = endpoint->desc.bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
+	i = d->bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
 	if (i >= 16 || i == 0) {
-		dev_err(ddev, "config %d interface %d altsetting %d has an "
-		    "invalid endpoint with address 0x%X\n",
-		    cfgno, inum, asnum, endpoint->desc.bEndpointAddress);
-		return -EINVAL;
+		dev_warn(ddev, "config %d interface %d altsetting %d has an "
+		    "invalid endpoint with address 0x%X, skipping\n",
+		    cfgno, inum, asnum, d->bEndpointAddress);
+		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
-	le16_to_cpus(&endpoint->desc.wMaxPacketSize);
+	/* Only store as many endpoints as we have room for */
+	if (ifp->desc.bNumEndpoints >= num_ep)
+		goto skip_to_next_endpoint_or_interface_descriptor;
 
-	buffer += header->bLength;
-	size -= header->bLength;
+	endpoint = &ifp->endpoint[ifp->desc.bNumEndpoints];
+	++ifp->desc.bNumEndpoints;
+
+	memcpy(&endpoint->desc, d, n);
+	le16_to_cpus(&endpoint->desc.wMaxPacketSize);
 
 	/* Skip over any Class Specific or Vendor Specific descriptors;
 	 * find the next endpoint or interface descriptor */
@@ -91,8 +96,13 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
 	    USB_DT_INTERFACE, &n);
 	endpoint->extralen = i;
 	if (n > 0)
-		dev_dbg(ddev, "skipped %d class/vendor specific endpoint "
-		    "descriptors\n", n);
+		dev_dbg(ddev, "skipped %d descriptor%s after %s\n",
+		    n, plural(n), "endpoint");
+	return buffer - buffer0 + i;
+
+skip_to_next_endpoint_or_interface_descriptor:
+	i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
+	    USB_DT_INTERFACE, NULL);
 	return buffer - buffer0 + i;
 }
 
@@ -107,7 +117,8 @@ static void usb_release_interface_cache(struct kref *ref)
 }
 
 static int usb_parse_interface(struct device *ddev, int cfgno,
-    struct usb_host_config *config, unsigned char *buffer, int size)
+    struct usb_host_config *config, unsigned char *buffer, int size,
+    u8 inums[], u8 nalts[])
 {
 	unsigned char *buffer0 = buffer;
 	struct usb_interface_descriptor	*d;
@@ -116,37 +127,41 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 	struct usb_host_interface *alt;
 	int i, n;
 	int len, retval;
+	int num_ep, num_ep_orig;
 
 	d = (struct usb_interface_descriptor *) buffer;
 	buffer += d->bLength;
 	size -= d->bLength;
 
-	if (d->bDescriptorType != USB_DT_INTERFACE) {
-		dev_err(ddev, "config %d has an unexpected descriptor of type "
-		    "0x%X, expecting interface type 0x%X\n",
-		    cfgno, d->bDescriptorType, USB_DT_INTERFACE);
-		return -EINVAL;
-	}
+	if (d->bLength < USB_DT_INTERFACE_SIZE)
+		goto skip_to_next_interface_descriptor;
 
+	/* Which interface entry is this? */
+	intfc = NULL;
 	inum = d->bInterfaceNumber;
-	if (inum >= config->desc.bNumInterfaces)
+	for (i = 0; i < config->desc.bNumInterfaces; ++i) {
+		if (inums[i] == inum) {
+			intfc = config->intf_cache[i];
+			break;
+		}
+	}
+	if (!intfc || intfc->num_altsetting >= nalts[i])
 		goto skip_to_next_interface_descriptor;
 
-	intfc = config->intf_cache[inum];
+	/* Check for duplicate altsetting entries */
 	asnum = d->bAlternateSetting;
-	if (asnum >= intfc->num_altsetting) {
-		dev_err(ddev, "config %d interface %d has an invalid "
-		    "alternate setting number: %d but max is %d\n",
-		    cfgno, inum, asnum, intfc->num_altsetting - 1);
-		return -EINVAL;
+	for ((i = 0, alt = &intfc->altsetting[0]);
+	      i < intfc->num_altsetting;
+	     (++i, ++alt)) {
+		if (alt->desc.bAlternateSetting == asnum) {
+			dev_warn(ddev, "Duplicate descriptor for config %d "
+			    "interface %d altsetting %d, skipping\n",
+			    cfgno, inum, asnum);
+			goto skip_to_next_interface_descriptor;
 		}
-
-	alt = &intfc->altsetting[asnum];
-	if (alt->desc.bLength) {
-		dev_err(ddev, "Duplicate descriptor for config %d "
-		    "interface %d altsetting %d\n", cfgno, inum, asnum);
-		return -EINVAL;
 	}
+
+	++intfc->num_altsetting;
 	memcpy(&alt->desc, d, USB_DT_INTERFACE_SIZE);
 
 	/* Skip over any Class Specific or Vendor Specific descriptors;
@@ -156,41 +171,48 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 	    USB_DT_INTERFACE, &n);
 	alt->extralen = i;
 	if (n > 0)
-		dev_dbg(ddev, "skipped %d class/vendor specific "
-		    "interface descriptors\n", n);
+		dev_dbg(ddev, "skipped %d descriptor%s after %s\n",
+		    n, plural(n), "interface");
 	buffer += i;
 	size -= i;
 
-	if (alt->desc.bNumEndpoints > USB_MAXENDPOINTS) {
-		dev_err(ddev, "too many endpoints for config %d interface %d "
-		    "altsetting %d: %d, maximum allowed: %d\n",
-		    cfgno, inum, asnum, alt->desc.bNumEndpoints,
-		    USB_MAXENDPOINTS);
-		return -EINVAL;
+	/* Allocate space for the right(?) number of endpoints */
+	num_ep = num_ep_orig = alt->desc.bNumEndpoints;
+	alt->desc.bNumEndpoints = 0;		// Use as a counter
+	if (num_ep > USB_MAXENDPOINTS) {
+		dev_warn(ddev, "too many endpoints for config %d interface %d "
+		    "altsetting %d: %d, using maximum allowed: %d\n",
+		    cfgno, inum, asnum, num_ep, USB_MAXENDPOINTS);
+		num_ep = USB_MAXENDPOINTS;
 	}
 
-	len = alt->desc.bNumEndpoints * sizeof(struct usb_host_endpoint);
+	len = sizeof(struct usb_host_endpoint) * num_ep;
 	alt->endpoint = kmalloc(len, GFP_KERNEL);
 	if (!alt->endpoint)
 		return -ENOMEM;
 	memset(alt->endpoint, 0, len);
 
-	for (i = 0; i < alt->desc.bNumEndpoints; i++) {
-		if (size < USB_DT_ENDPOINT_SIZE) {
-			dev_err(ddev, "too few endpoint descriptors for "
-			    "config %d interface %d altsetting %d\n",
-			    cfgno, inum, asnum);
-			return -EINVAL;
-		}
-
-		retval = usb_parse_endpoint(ddev, cfgno, inum, asnum,
-		    alt->endpoint + i, buffer, size);
+	/* Parse all the endpoint descriptors */
+	n = 0;
+	while (size > 0) {
+		if (((struct usb_descriptor_header *) buffer)->bDescriptorType
+		     == USB_DT_INTERFACE)
+			break;
+		retval = usb_parse_endpoint(ddev, cfgno, inum, asnum, alt,
+		    num_ep, buffer, size);
 		if (retval < 0)
 			return retval;
+		++n;
 
 		buffer += retval;
 		size -= retval;
 	}
+
+	if (n != num_ep_orig)
+		dev_warn(ddev, "config %d interface %d altsetting %d has %d "
+		    "endpoint descriptor%s, different from the interface "
+		    "descriptor's value: %d\n",
+		    cfgno, inum, asnum, n, plural(n), num_ep_orig);
 	return buffer - buffer0;
 
 skip_to_next_interface_descriptor:
@@ -202,6 +224,7 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 int usb_parse_configuration(struct device *ddev, int cfgidx,
     struct usb_host_config *config, unsigned char *buffer, int size)
 {
+	unsigned char *buffer0 = buffer;
 	int cfgno;
 	int nintf, nintf_orig;
 	int i, j, n;
@@ -210,7 +233,7 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 	int size2;
 	struct usb_descriptor_header *header;
 	int len, retval;
-	u8 nalts[USB_MAXINTERFACES];
+	u8 inums[USB_MAXINTERFACES], nalts[USB_MAXINTERFACES];
 
 	memcpy(&config->desc, buffer, USB_DT_CONFIG_SIZE);
 	if (config->desc.bDescriptorType != USB_DT_CONFIG ||
@@ -220,7 +243,6 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 		    config->desc.bDescriptorType, config->desc.bLength);
 		return -EINVAL;
 	}
-	config->desc.wTotalLength = size;
 	cfgno = config->desc.bConfigurationValue;
 
 	buffer += config->desc.bLength;
@@ -231,68 +253,102 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 		dev_warn(ddev, "config %d has too many interfaces: %d, "
 		    "using maximum allowed: %d\n",
 		    cfgno, nintf, USB_MAXINTERFACES);
-		config->desc.bNumInterfaces = nintf = USB_MAXINTERFACES;
+		nintf = USB_MAXINTERFACES;
 	}
-	memset(nalts, 0, nintf);
 
 	/* Go through the descriptors, checking their length and counting the
 	 * number of altsettings for each interface */
+	n = 0;
 	for ((buffer2 = buffer, size2 = size);
-	      size2 >= sizeof(struct usb_descriptor_header);
+	      size2 > 0;
 	     (buffer2 += header->bLength, size2 -= header->bLength)) {
 
+		if (size2 < sizeof(struct usb_descriptor_header)) {
+			dev_warn(ddev, "config %d descriptor has %d excess "
+			    "byte%s, ignoring\n",
+			    cfgno, size2, plural(size2));
+			break;
+		}
+
 		header = (struct usb_descriptor_header *) buffer2;
 		if ((header->bLength > size2) || (header->bLength < 2)) {
-			dev_err(ddev, "config %d has an invalid descriptor "
-			    "of length %d\n", cfgno, header->bLength);
-			return -EINVAL;
+			dev_warn(ddev, "config %d has an invalid descriptor "
+			    "of length %d, skipping remainder of the config\n",
+			    cfgno, header->bLength);
+			break;
 		}
 
 		if (header->bDescriptorType == USB_DT_INTERFACE) {
 			struct usb_interface_descriptor *d;
+			int inum;
 
 			d = (struct usb_interface_descriptor *) header;
 			if (d->bLength < USB_DT_INTERFACE_SIZE) {
-				dev_err(ddev, "config %d has an invalid "
-				    "interface descriptor of length %d\n",
-				    cfgno, d->bLength);
-				return -EINVAL;
+				dev_warn(ddev, "config %d has an invalid "
+				    "interface descriptor of length %d, "
+				    "skipping\n", cfgno, d->bLength);
+				continue;
 			}
 
-			i = d->bInterfaceNumber;
-			if (i >= nintf_orig) {
-				dev_err(ddev, "config %d has an invalid "
+			inum = d->bInterfaceNumber;
+			if (inum >= nintf_orig)
+				dev_warn(ddev, "config %d has an invalid "
 				    "interface number: %d but max is %d\n",
-				    cfgno, i, nintf_orig - 1);
-				return -EINVAL;
+				    cfgno, inum, nintf_orig - 1);
+
+			/* Have we already encountered this interface?
+			 * Count its altsettings */
+			for (i = 0; i < n; ++i) {
+				if (inums[i] == inum)
+					break;
 			}
-			if (i < nintf && nalts[i] < 255)
+			if (i < n) {
+				if (nalts[i] < 255)
 					++nalts[i];
+			} else if (n < USB_MAXINTERFACES) {
+				inums[n] = inum;
+				nalts[n] = 1;
+				++n;
+			}
 
 		} else if (header->bDescriptorType == USB_DT_DEVICE ||
-			    header->bDescriptorType == USB_DT_CONFIG) {
-			dev_err(ddev, "config %d contains an unexpected "
-			    "descriptor of type 0x%X\n",
+			    header->bDescriptorType == USB_DT_CONFIG)
+			dev_warn(ddev, "config %d contains an unexpected "
+			    "descriptor of type 0x%X, skipping\n",
 			    cfgno, header->bDescriptorType);
-			return -EINVAL;
-		}
 
 	}	/* for ((buffer2 = buffer, size2 = size); ...) */
+	size = buffer2 - buffer;
+	config->desc.wTotalLength = buffer2 - buffer0;
+
+	if (n != nintf)
+		dev_warn(ddev, "config %d has %d interface%s, different from "
+		    "the descriptor's value: %d\n",
+		    cfgno, n, plural(n), nintf_orig);
+	else if (n == 0)
+		dev_warn(ddev, "config %d has no interfaces?\n", cfgno);
+	config->desc.bNumInterfaces = nintf = n;
+
+	/* Check for missing interface numbers */
+	for (i = 0; i < nintf; ++i) {
+		for (j = 0; j < nintf; ++j) {
+			if (inums[j] == i)
+				break;
+		}
+		if (j >= nintf)
+			dev_warn(ddev, "config %d has no interface number "
+			    "%d\n", cfgno, i);
+	}
 
 	/* Allocate the usb_interface_caches and altsetting arrays */
 	for (i = 0; i < nintf; ++i) {
 		j = nalts[i];
 		if (j > USB_MAXALTSETTING) {
-			dev_err(ddev, "too many alternate settings for "
+			dev_warn(ddev, "too many alternate settings for "
 			    "config %d interface %d: %d, "
-			    "maximum allowed: %d\n",
-			    cfgno, i, j, USB_MAXALTSETTING);
-			return -EINVAL;
-		}
-		if (j == 0) {
-			dev_err(ddev, "config %d has no interface number "
-			    "%d\n", cfgno, i);
-			return -EINVAL;
+			    "using maximum allowed: %d\n",
+			    cfgno, inums[i], j, USB_MAXALTSETTING);
+			nalts[i] = j = USB_MAXALTSETTING;
 		}
 
 		len = sizeof(*intfc) + sizeof(struct usb_host_interface) * j;
@@ -300,7 +356,6 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 		if (!intfc)
 			return -ENOMEM;
 		memset(intfc, 0, len);
-		intfc->num_altsetting = j;
 		kref_init(&intfc->ref, usb_release_interface_cache);
 	}
 
@@ -311,15 +366,15 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 	    USB_DT_INTERFACE, &n);
 	config->extralen = i;
 	if (n > 0)
-		dev_dbg(ddev, "skipped %d class/vendor specific "
-		    "configuration descriptors\n", n);
+		dev_dbg(ddev, "skipped %d descriptor%s after %s\n",
+		    n, plural(n), "configuration");
 	buffer += i;
 	size -= i;
 
 	/* Parse all the interface/altsetting descriptors */
-	while (size >= sizeof(struct usb_descriptor_header)) {
+	while (size > 0) {
 		retval = usb_parse_interface(ddev, cfgno, config,
-		    buffer, size);
+		    buffer, size, inums, nalts);
 		if (retval < 0)
 			return retval;
 
@@ -331,15 +386,18 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 	for (i = 0; i < nintf; ++i) {
 		intfc = config->intf_cache[i];
 		for (j = 0; j < intfc->num_altsetting; ++j) {
-			if (!intfc->altsetting[j].desc.bLength) {
-				dev_err(ddev, "config %d interface %d has no "
-				    "altsetting %d\n", cfgno, i, j);
-				return -EINVAL;
+			for (n = 0; n < intfc->num_altsetting; ++n) {
+				if (intfc->altsetting[n].desc.
+				    bAlternateSetting == j)
+					break;
 			}
+			if (n >= intfc->num_altsetting)
+				dev_warn(ddev, "config %d interface %d has no "
+				    "altsetting %d\n", cfgno, inums[i], j);
 		}
 	}
 
-	return size;
+	return 0;
 }
 
 // hub-only!! ... and only exported for reset/reinit path.
@@ -445,21 +503,16 @@ int usb_get_configuration(struct usb_device *dev)
 			goto err;
 		}
 		if (result < length) {
-			dev_err(ddev, "config index %d descriptor too short "
+			dev_warn(ddev, "config index %d descriptor too short "
 			    "(expected %i, got %i)\n", cfgno, length, result);
-			result = -EINVAL;
-			kfree(bigbuffer);
-			goto err;
+			length = result;
 		}
 
 		dev->rawdescriptors[cfgno] = bigbuffer;
 
 		result = usb_parse_configuration(&dev->dev, cfgno,
 		    &dev->config[cfgno], bigbuffer, length);
-		if (result > 0)
-			dev_dbg(ddev, "config index %d descriptor has %d "
-			    "excess byte(s)\n", cfgno, result);
-		else if (result < 0) {
+		if (result < 0) {
 			++cfgno;
 			goto err;
 		}