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@xxxxxxxxxxx>
> 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
