| Message ID | CAL6iAa+Pd83Z0zA-26cvEbMxmH7r0QbUzi9XZowoEiZ8emZv+Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 20, 2017 at 6:57 AM, Kim Jaejoong <climbbb.kim@gmail.com> wrote: > Hi Andrey > > 2017-09-19 21:38 GMT+09:00 Andrey Konovalov <andreyknvl@google.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@google.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); > ----------- > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 20 Sep 2017, Kim Jaejoong wrote: > 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? The descriptors can appear in any order. You should not assume that the report descriptor will always come first. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan 2017-09-21 0:50 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>: > On Wed, 20 Sep 2017, Kim Jaejoong wrote: > >> 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? > > The descriptors can appear in any order. You should not assume that > the report descriptor will always come first. Thanks for clarifying. I will resend patch with modification. Jaejoong > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);