Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751878AbdITKtd (ORCPT ); Wed, 20 Sep 2017 06:49:33 -0400 Received: from mail-oi0-f49.google.com ([209.85.218.49]:47665 "EHLO mail-oi0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751716AbdITKta (ORCPT ); Wed, 20 Sep 2017 06:49:30 -0400 X-Google-Smtp-Source: AOwi7QC+foliNYKFydduuQATy8OevuYueSMCdL8KPG2X3e9Op39m876VTf8ZvWf7krFPYtBiMGIBtKPkZLLQMvaNmXc= MIME-Version: 1.0 In-Reply-To: References: From: Andrey Konovalov Date: Wed, 20 Sep 2017 12:49:28 +0200 Message-ID: Subject: Re: usb/hid: slab-out-of-bounds read in usbhid_parse To: Kim Jaejoong Cc: Jiri Kosina , Benjamin Tissoires , USB list , linux-input@vger.kernel.org, LKML , syzkaller , Dmitry Vyukov , Kostya Serebryany Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3022 Lines: 87 On Wed, Sep 20, 2017 at 6:57 AM, Kim Jaejoong wrote: > Hi Andrey > > 2017-09-19 21:38 GMT+09:00 Andrey Konovalov : >> 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 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); > ----------- >