linux-kernel - Re: usb/hid: slab-out-of-bounds read in usbhid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAeHK+yELqVWqUrk+y0Mm9ZwKc=ScSt886Y6hQ144O_-HKWYLQ@mail.gmail.com>
Date:   Wed, 20 Sep 2017 12:49:28 +0200
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Kim Jaejoong <climbbb.kim@...il.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        USB list <linux-usb@...r.kernel.org>,
        linux-input@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        syzkaller <syzkaller@...glegroups.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>
Subject: Re: usb/hid: slab-out-of-bounds read in usbhid_parse

On Wed, Sep 20, 2017 at 6:57 AM, Kim Jaejoong <climbbb.kim@...il.com> wrote:
> Hi Andrey
>
> 2017-09-19 21:38 GMT+09:00 Andrey Konovalov <andreyknvl@...gle.com>:
>> 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@...gle.com>

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);
> -----------
>