Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970289AbdIZT75 (ORCPT ); Tue, 26 Sep 2017 15:59:57 -0400 Received: from mail-io0-f180.google.com ([209.85.223.180]:56257 "EHLO mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965209AbdIZT7y (ORCPT ); Tue, 26 Sep 2017 15:59:54 -0400 X-Google-Smtp-Source: AOwi7QCe4i+lYMM66pKswxU43hh100kUiWNPCs5I34ONUauCMYT3YNL3wI/JIamhdEkTENWAb4fqKzfh3Uje54E/fYA= MIME-Version: 1.0 In-Reply-To: References: <1506411084-7324-1-git-send-email-climbbb.kim@gmail.com> From: Jaejoong Kim Date: Wed, 27 Sep 2017 04:59:52 +0900 Message-ID: Subject: Re: [PATCH] HID: usbhid: fix out-of-bounds bug To: Alan Stern Cc: Jiri Kosina , Andrey Konovalov , 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: 6715 Lines: 172 Hi, Alan, Thanks for the review. 2017-09-26 23:18 GMT+09:00 Alan Stern : > On Tue, 26 Sep 2017, Jaejoong Kim wrote: > >> The starting address of the hid descriptor is obtained via >> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it >> is possible to access the wrong address. So, before accessing the hid >> descriptor, we need to check the entire size through the bLength field. >> >> It also shows how many class descriptors it has through the bNumDescriptors >> of the hid descriptor. Assuming that the connected hid descriptor has two >> class descriptors(report and physical descriptors), the code below can >> cause OOB because hdesc->desc is an array of size 1. >> >> for (n = 0; n < hdesc->bNumDescriptors; n++) >> if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT) >> rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength); >> >> Since we know the starting address of the hid descriptor and the value of >> the bNumDescriptors is variable, we directly access the buffer containing >> the hid descriptor without usbing hdesc->desc to obtain the size of the >> report descriptor. > > I do not like this patch at all. > >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >> index 089bad8..7bad173 100644 >> --- a/drivers/hid/usbhid/hid-core.c >> +++ b/drivers/hid/usbhid/hid-core.c >> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid) >> struct usb_interface *intf = to_usb_interface(hid->dev.parent); >> struct usb_host_interface *interface = intf->cur_altsetting; >> struct usb_device *dev = interface_to_usbdev (intf); >> - struct hid_descriptor *hdesc; >> + unsigned char *buffer = intf->altsetting->extra; >> + int buflen = intf->altsetting->extralen; >> + int length; >> u32 quirks = 0; >> unsigned int rsize = 0; >> char *rdesc; >> int ret, n; >> >> + if (!buffer) { >> + dbg_hid("class descriptor not present\n"); >> + return -ENODEV; >> + } >> + >> quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor), >> le16_to_cpu(dev->descriptor.idProduct)); >> >> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid) >> quirks |= HID_QUIRK_NOGET; >> } >> >> - if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) && >> - (!interface->desc.bNumEndpoints || >> - usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) { >> - dbg_hid("class descriptor not present\n"); >> - return -ENODEV; >> - } >> + while (buflen > 2) { >> + length = buffer[0]; >> + if (!length || length < HID_DESCRIPTOR_MIN_SIZE) >> + goto next_desc; > > It's silly to duplicate the code that is already present in > usb_get_extra_descriptor(). There's no reason to avoid using that > function here. To be honest, I was fooled. You are right. That is a duplicate code with usb_get_extra_descriptor(). > >> - hid->version = le16_to_cpu(hdesc->bcdHID); >> - hid->country = hdesc->bCountryCode; >> + if (buffer[1] == HID_DT_HID) { >> + hid->version = get_unaligned_le16(&buffer[2]); >> + hid->country = buffer[4]; > > It's also silly not to use the preformatted hid_descriptor structure and > instead put error-prone byte offsets directly into the code. Right. > >> - for (n = 0; n < hdesc->bNumDescriptors; n++) >> - if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT) >> - rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength); >> + for (n = 0; n < buffer[5]; n++) { >> + /* we are just interested in report descriptor */ >> + if (buffer[6+3*n] != HID_DT_REPORT) >> + continue; >> + rsize = get_unaligned_le16(&buffer[7+3*n]); > > Finally, this patch doesn't fix the actual problem! You don't check > here whether 8+3*n >= length. > > This whole thing could be fixed with a much smaller change to the > original code. Just do something like this: > > num_descriptors = min_t(int, hdesc->bNumDescriptors, > (hdesc->bLength - 6) / 3); > for (n = 0; n < num_descriptors; n++) Right. Your change is better and clear. With your change, I think we need check the size of hdesc before accessing hdesc->desc[n] as Andrey Konovalov has already pointed out, diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 089bad8..0b41d44 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -975,6 +975,7 @@ static int usbhid_parse(struct hid_device *hid) unsigned int rsize = 0; char *rdesc; int ret, n; + int num_descriptors; quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor), le16_to_cpu(dev->descriptor.idProduct)); @@ -997,10 +998,17 @@ static int usbhid_parse(struct hid_device *hid) return -ENODEV; } + if (hdesc->bLength < sizeof(struct hid_descriptor)) { + 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++) + num_descriptors = min_t(int, hdesc->bNumDescriptors, + (hdesc->bLength - 6) / 3); + for (n = 0; n < num_descriptors; n++) if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT) rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength); Jaejoong > > Alan Stern > >> + } >> + } >> + >> +next_desc: >> + buflen -= length; >> + buffer += length; >> + } >> >> if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) { >> dbg_hid("weird size of report descriptor (%u)\n", rsize); >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index ab05a86..2d53c0f 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -638,6 +638,8 @@ struct hid_descriptor { >> struct hid_class_descriptor desc[1]; >> } __attribute__ ((packed)); >> >> +#define HID_DESCRIPTOR_MIN_SIZE 9 >> + >> #define HID_DEVICE(b, g, ven, prod) \ >> .bus = (b), .group = (g), .vendor = (ven), .product = (prod) >> #define HID_USB_DEVICE(ven, prod) \ >> >