| Message ID | CAEFJ+1OA+=SBRa_O3UupJT_KHm_XDw-4uJ9J3cRJQOW0T_c6aA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lack of length checking in USB configuration may allow buffer overflow | expand |
On Mon, May 13, 2019 at 07:14:38PM -0700, Rick Mark wrote: > 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, Thanks for the patch, but your email client ate all of the tabs and line-wrapped it, making it impossible to apply, even if we wanted to :) Also, I need a lot better changelog text and description, as well as a signed-off-by line. Documentation/SubmittingPatches should explain all of how to do this. If you have questions after reading this, please let me know. That being said, I don't think all of your patch is really needed: > > /* 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; > + } How can size be smaller than this, I think we check this value before calling this function in all places. Did we miss one? > + > 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; > + } A patch was just sent to the list to resolve a problem in this function yesterday, can you verify that it resolves your issue as well? > + > 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. > */ We do this same check the line below this, why do it twice? > @@ -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; > + } > + Didn't we just check this above here successfully? I didn't go through all of your others, but please be sure that we are not already doing all of this, as I think we are. Are you sure you are using the latest kernel version? thanks, greg k-h
On Mon, 13 May 2019, Rick Mark wrote: > Hey All, > > I was seeing a linux VM crash due to malformed USB configuration > payloads being malformed. Can you provide more information about this crash? I would like to know exactly what errors are occurring. As far as I can tell, the existing code already tests for all the things your patch adds. > 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.) Have you read the loop in usb_parse_configuration() that starts at the comment: /* Go through the descriptors, checking their length and counting the * number of altsettings for each interface */ (approximately line 585)? This loop should carry out all the tests that your patch is trying to duplicate. Alan Stern
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)
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(+) 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;