Re: Lack of length checking in USB configuration may allow buffer overflow

[Greg KH <gregkh@linuxfoundation.org>]

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