Lack of length checking in USB configuration may allow buffer overflow

* Lack of length checking in USB configuration may allow buffer overflow
@ 2019-05-14  2:14 Rick Mark
  2019-05-14  7:14 ` Greg KH
  2019-05-14 13:38 ` Alan Stern
  0 siblings, 2 replies; 3+ messages in thread
From: Rick Mark @ 2019-05-14  2:14 UTC (permalink / raw)
  To: linux-usb; +Cc: enterprise-infrasec-team, production-infrasec-team

Hey All,

I was seeing a linux VM crash due to malformed USB configuration
payloads being malformed.  I'm testing this patch now which should
provide better security checking (but this is my first patch so be
kind if I have things wrong.)

R

From d7b0dd52f3b3b38126504b17d2d9c9ceaa572edf Mon Sep 17 00:00:00 2001
From: Rick Mark <rickmark@outlook.com>
Date: Mon, 13 May 2019 19:06:46 -0700
Subject: [PATCH] Security checks in USB configurations

---
 drivers/usb/core/config.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 7b5cb28ff..8cb9a136e 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -33,6 +33,13 @@ static int find_next_descriptor(unsigned char
*buffer, int size,

  /* Find the next descriptor of type dt1 or dt2 */
  while (size > 0) {
+     if (size < sizeof(struct usb_descriptor_header)) {
+         printk( KERN_ERR "usb config: find_next_descriptor "
+                          "with size %d not sizeof("
+                          "struct usb_descriptor_header)", size );
+         break;
+     }
+
  h = (struct usb_descriptor_header *) buffer;
  if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
  break;
@@ -58,6 +65,13 @@ static void
usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
  * The SuperSpeedPlus Isoc endpoint companion descriptor immediately
  * follows the SuperSpeed Endpoint Companion descriptor
  */
+ if (size < sizeof(struct usb_ssp_isoc_ep_comp_descriptor)) {
+        dev_warn(ddev, "Invalid size %d for SuperSpeedPlus isoc
endpoint companion"
+                       "for config %d interface %d altsetting %d ep %d.\n",
+                 size, cfgno, inum, asnum, ep->desc.bEndpointAddress);
+        return;
+ }
+
  desc = (struct usb_ssp_isoc_ep_comp_descriptor *) buffer;
  if (desc->bDescriptorType != USB_DT_SSP_ISOC_ENDPOINT_COMP ||
      size < USB_DT_SSP_ISOC_EP_COMP_SIZE) {
@@ -76,6 +90,14 @@ static void usb_parse_ss_endpoint_companion(struct
device *ddev, int cfgno,
  struct usb_ss_ep_comp_descriptor *desc;
  int max_tx;

+ if (size < sizeof(struct usb_ss_ep_comp_descriptor)) {
+        dev_warn(ddev, "Invalid size %d of SuperSpeed endpoint"
+                       " companion for config %d "
+                       " interface %d altsetting %d: "
+                       "using minimum values\n",
+                 size, cfgno, inum, asnum);
+        return;
+ }
  /* The SuperSpeed endpoint companion descriptor is supposed to
  * be the first thing immediately following the endpoint descriptor.
  */
@@ -103,6 +125,16 @@ static void
usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
  ep->desc.wMaxPacketSize;
  return;
  }
+
+ if ((size - desc->bLength) < 0 ||
+     size < USB_DT_SS_EP_COMP_SIZE) {
+        dev_warn(ddev, "Control endpoint with bMaxBurst = %d in "
+                       "config %d interface %d altsetting %d ep %d: "
+                       "has invalid bLength %d vs size %d\n", desc->bMaxBurst,
+                 cfgno, inum, asnum, ep->desc.bEndpointAddress,
desc->bLength, size);
+        return;
+ }
+
  buffer += desc->bLength;
  size -= desc->bLength;
  memcpy(&ep->ss_ep_comp, desc, USB_DT_SS_EP_COMP_SIZE);
@@ -214,7 +246,24 @@ static int usb_parse_endpoint(struct device
*ddev, int cfgno, int inum,
  unsigned int maxp;
  const unsigned short *maxpacket_maxes;

+ if (size < sizeof(struct usb_endpoint_descriptor)) {
+        dev_warn(ddev, "config %d interface %d altsetting %d has an "
+                       "size %d smaller then endpoint descriptor, skipping\n",
+                 cfgno, inum, asnum, size);
+
+        return -EINVAL;
+ }
+
  d = (struct usb_endpoint_descriptor *) buffer;
+
+ if ((size - d->bLength) < 0) {
+        dev_warn(ddev, "config %d interface %d altsetting %d has an "
+                       "invalid endpoint descriptor of length %d, skipping\n",
+                 cfgno, inum, asnum, d->bLength);
+
+        return -EINVAL;
+ }
+
  buffer += d->bLength;
  size -= d->bLength;

@@ -446,7 +495,18 @@ static int usb_parse_interface(struct device
*ddev, int cfgno,
  int len, retval;
  int num_ep, num_ep_orig;

+ if (size < sizeof(struct usb_interface_descriptor)) {
+        dev_err(ddev, "config %d interface %d has an "
+                       "invalid endpoint descriptor of length %d, skipping\n",
+                 cfgno, inum, size);
+    }
  d = (struct usb_interface_descriptor *) buffer;
+
+ if ((size - d->bLength) < 0) {
+        dev_err(ddev, "config %d interface %d has an "
+                       "invalid endpoint descriptor of length %d, skipping\n",
+                 cfgno, inum, d->bLength);
+ }
  buffer += d->bLength;
  size -= d->bLength;

@@ -514,6 +574,13 @@ static int usb_parse_interface(struct device
*ddev, int cfgno,
  /* Parse all the endpoint descriptors */
  n = 0;
  while (size > 0) {
+     if (size < sizeof(struct usb_descriptor_header)) {
+            dev_err(ddev, "config %d interface %d has an "
+                           "invalid endpoint descriptor of length %d,
skipping\n",
+                     cfgno, inum, size);
+            return -EINVAL;
+     }
+
  if (((struct usb_descriptor_header *) buffer)->bDescriptorType
       == USB_DT_INTERFACE)
  break;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread