Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Andrey Konovalov]

On Wed, Sep 20, 2017 at 6:57 AM, Kim Jaejoong <climbbb.kim@xxxxxxxxx> wrote:
> Hi Andrey
>
> 2017-09-19 21:38 GMT+09:00 Andrey Konovalov <andreyknvl@xxxxxxxxxx>:
>> Hi Kim,
>>
>> I'm not sure. Is there a check on the bLength field of a
>> hid_descriptor struct? Can it be less than sizeof(struct
>> hid_descriptor)? If so, we still do an out-of-bounds access to
>> hdesc->desc[0] (or some other fields).
>
> You are right. I add hid descriptr size from HID device with bLength field.
>
> Could you test and review below patch?

It seems that this patch fixes the issue. I'll keep fuzzing to make sure.

Tested-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>

Thanks!

>
> To. usb & input guys.
>
> While dig this report, i was wondering about bNumDescriptors in HID descriptor.
> HID document from usb.org said, 'this number must be at least one (1)
> as a Report descriptor will always be present.'
>
> There is no mention of the order of class descriptors. Suppose you
> have a HID device with a report descriptor and a physical descriptor.
>
> If you have the following hid descriptor in this case,
> HID descriptor
>    bLength: 12
>    bDescriptor Type: HID
>    .. skip
>    bNumDescriptors: 2
>    bDescriptorType: physical
>    bDescriptorLength: any
>    bDescriptorType: Report
>    bDescriptorLength: any
>
> If the order of the report descriptor is the second as above,
> usbhid_parse () will fail because my patch is only check the first
> bDescriptor Type.
> But If the order of the report descriptor is always first, there is no
> problem. How do you think this?
>
> Thanks, jaejoong
>
> -----------------
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..94c3805 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
>         u32 quirks = 0;
>         unsigned int rsize = 0;
>         char *rdesc;
> -       int ret, n;
> +       int ret;
>
>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>                         le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,12 +997,16 @@ static int usbhid_parse(struct hid_device *hid)
>                 return -ENODEV;
>         }
>
> +       if (hdesc->bLength < sizeof(*hdesc)) {
> +               dbg_hid("hid descriptor is too short\n");
> +               return -EINVAL;
> +       }
> +
>         hid->version = le16_to_cpu(hdesc->bcdHID);
>         hid->country = hdesc->bCountryCode;
>
> -       for (n = 0; n < hdesc->bNumDescriptors; n++)
> -               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> -                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> +       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
> +               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);
>
>         if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>                 dbg_hid("weird size of report descriptor (%u)\n", rsize);
> -----------
>