From patchwork Tue Sep 26 19:59:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jaejoong Kim X-Patchwork-Id: 9972615 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E023C6037F for ; Tue, 26 Sep 2017 19:59:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CBDCD28C3D for ; Tue, 26 Sep 2017 19:59:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BE65028F1B; Tue, 26 Sep 2017 19:59:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 053EB2894F for ; Tue, 26 Sep 2017 19:59:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968636AbdIZT74 (ORCPT ); Tue, 26 Sep 2017 15:59:56 -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 Received: by mail-io0-f180.google.com with SMTP id z187so13783664ioz.12; Tue, 26 Sep 2017 12:59:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fzms/Erx4SxqMdmE0PqXESz/Mt++Mj/DAfmRbpF2fxw=; b=A8++iM5i7WWSvTNkp+OrMKsd8pM+nm3+3B1+9+as2vRwJtEAV2Q4Isusi3T6lVWdZ/ zuYI7nUFt/6n+eqrPmkzSJcrRij4HrQd6eFLLxo/AFS7uHqZufccCBcJEV+QqNf7MZwU DJrOCI7WZzF/JRG7WfpIgUHUBkQScm0PQuNvnz/d8eTgIKCB/tYwdMWizHBfs5oP4Ebl YF2cPaUeo29Me5RGeiZ5zaxhZ7+iQ3jyEA44vg2PjG+7Pqh9Ve6RMA0gLQIuu+XkWJog ihW/IvS/lQ86WoSyTCzovUXtxna3yzxUXSLvXccXZ6OdG0s4lPRfKAzeBWiWwNx/bu6V Krfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fzms/Erx4SxqMdmE0PqXESz/Mt++Mj/DAfmRbpF2fxw=; b=QnJdSyrL2GXh4PcqAwaXDRV4mUkNNHU3bSqFc7sYrB0BggjbFy83JtO/7G0KAwjTBV fICS6zClFxwzQ2tDZ/h8olJMdgOgYS8AlryDtnKjfNv030pvh3MmMpXjTEmLTM4gOt/h ZtsqarEHWJq9QGo+RD3Kt/xqSqfLwZKJAkmbu0MtIYXmrlu6WmMHe5Vxk8wBf50qIOGk M6+dpIyjsYVVjMVWRh5+NzvtbxIjEIYd/cmj12YMFhp9pg381EwCo0XtNWykAHbxFsWb vjPQkgp1KMaRoNHtFZxco5yCWqFb+zWiSd1tXavJLtugfE0UsSv51np7hK7wzD75gSHG BCgA== X-Gm-Message-State: AHPjjUgD0R5jeW//2PmnGIsd2Uy86tGFpo7s6+vv6NYZ2xO4RvjS5+kF 4dUqxjUCYsbiE3BzWHVm1XPN8yECS4q+LA/y60qjuQ== X-Google-Smtp-Source: AOwi7QCe4i+lYMM66pKswxU43hh100kUiWNPCs5I34ONUauCMYT3YNL3wI/JIamhdEkTENWAb4fqKzfh3Uje54E/fYA= X-Received: by 10.107.15.32 with SMTP id x32mr15655881ioi.7.1506455993417; Tue, 26 Sep 2017 12:59:53 -0700 (PDT) MIME-Version: 1.0 Received: by 10.79.142.206 with HTTP; Tue, 26 Sep 2017 12:59:52 -0700 (PDT) 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 Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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, 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) \ >> > --- 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..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);