| 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
| ||
|
Message-ID:
<SN6PR02MB4157BC3FE0722EC3736E75AFD4D32@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Thu, 13 Mar 2025 20:25:31 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Terry Junge <linuxhid@...micgizmosystems.com>, Jiri Kosina
<jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>, Greg
Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Nikita Zhandarovich <n.zhandarovich@...tech.ru>, Alan Stern
<stern@...land.harvard.edu>, Kees Cook <kees@...nel.org>, "Gustavo A. R.
Silva" <gustavoars@...nel.org>, "linux-input@...r.kernel.org"
<linux-input@...r.kernel.org>, "linux-usb@...r.kernel.org"
<linux-usb@...r.kernel.org>, "linux-hyperv@...r.kernel.org"
<linux-hyperv@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-hardening@...r.kernel.org"
<linux-hardening@...r.kernel.org>, "syzkaller-bugs@...glegroups.com"
<syzkaller-bugs@...glegroups.com>, "lvc-project@...uxtesting.org"
<lvc-project@...uxtesting.org>,
"syzbot+c52569baf0c843f35495@...kaller.appspotmail.com"
<syzbot+c52569baf0c843f35495@...kaller.appspotmail.com>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v2] HID: usbhid: Eliminate recurrent out-of-bounds bug in
usbhid_parse()
From: Terry Junge <linuxhid@...micgizmosystems.com> Sent: Wednesday, March 12, 2025 3:24 PM
>
> Update struct hid_descriptor to better reflect the mandatory and
> optional parts of the HID Descriptor as per USB HID 1.11 specification.
> Note: the kernel currently does not parse any optional HID class
> descriptors, only the mandatory report descriptor.
>
> Update all references to member element desc[0] to rpt_desc.
>
> Add test to verify bLength and bNumDescriptors values are valid.
>
> Replace the for loop with direct access to the mandatory HID class
> descriptor member for the report descriptor. This eliminates the
> possibility of getting an out-of-bounds fault.
>
> Add a warning message if the HID descriptor contains any unsupported
> optional HID class descriptors.
>
> Reported-by: syzbot+c52569baf0c843f35495@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c52569baf0c843f35495
> Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
> Cc: stable@...r.kernel.org
> Signed-off-by: Terry Junge <linuxhid@...micgizmosystems.com>
> ---
> v1: Remove unnecessary for loop searching for the report descriptor size.
> v2: Fix compiler warning.
> base-commit: 58c9bf3363e596d744f56616d407278ef5f97f5a
>
> P.S. This is an alternative to the solution proposed by Nikita Zhandarovich
> <n.zhandarovich@...tech.ru>
> Link: https://lore.kernel.org/all/20250131151600.410242-1-n.zhandarovich@fintech.ru/
>
> include/linux/hid.h | 3 ++-
> drivers/usb/gadget/function/f_hid.c | 12 ++++++------
> drivers/hid/hid-hyperv.c | 4 ++--
> drivers/hid/usbhid/hid-core.c | 25 ++++++++++++++-----------
> 4 files changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cdc0dc13c87f..7abc8c74bdd5 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -738,8 +738,9 @@ struct hid_descriptor {
> __le16 bcdHID;
> __u8 bCountryCode;
> __u8 bNumDescriptors;
> + struct hid_class_descriptor rpt_desc;
>
> - struct hid_class_descriptor desc[1];
> + struct hid_class_descriptor opt_descs[];
> } __attribute__ ((packed));
>
> #define HID_DEVICE(b, g, ven, prod) \
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 740311c4fa24..c7a05f842745 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -144,8 +144,8 @@ static struct hid_descriptor hidg_desc = {
> .bcdHID = cpu_to_le16(0x0101),
> .bCountryCode = 0x00,
> .bNumDescriptors = 0x1,
> - /*.desc[0].bDescriptorType = DYNAMIC */
> - /*.desc[0].wDescriptorLenght = DYNAMIC */
> + /*.rpt_desc.bDescriptorType = DYNAMIC */
> + /*.rpt_desc.wDescriptorLength = DYNAMIC */
> };
>
> /* Super-Speed Support */
> @@ -939,8 +939,8 @@ static int hidg_setup(struct usb_function *f,
> struct hid_descriptor hidg_desc_copy = hidg_desc;
>
> VDBG(cdev, "USB_REQ_GET_DESCRIPTOR: HID\n");
> - hidg_desc_copy.desc[0].bDescriptorType = HID_DT_REPORT;
> - hidg_desc_copy.desc[0].wDescriptorLength =
> + hidg_desc_copy.rpt_desc.bDescriptorType = HID_DT_REPORT;
> + hidg_desc_copy.rpt_desc.wDescriptorLength =
> cpu_to_le16(hidg->report_desc_length);
>
> length = min_t(unsigned short, length,
> @@ -1210,8 +1210,8 @@ static int hidg_bind(struct usb_configuration *c, struct
> usb_function *f)
> * We can use hidg_desc struct here but we should not relay
> * that its content won't change after returning from this function.
> */
> - hidg_desc.desc[0].bDescriptorType = HID_DT_REPORT;
> - hidg_desc.desc[0].wDescriptorLength =
> + hidg_desc.rpt_desc.bDescriptorType = HID_DT_REPORT;
> + hidg_desc.rpt_desc.wDescriptorLength =
> cpu_to_le16(hidg->report_desc_length);
>
> hidg_hs_in_ep_desc.bEndpointAddress =
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> index 0fb210e40a41..9eafff0b6ea4 100644
> --- a/drivers/hid/hid-hyperv.c
> +++ b/drivers/hid/hid-hyperv.c
> @@ -192,7 +192,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> goto cleanup;
>
> input_device->report_desc_size = le16_to_cpu(
> - desc->desc[0].wDescriptorLength);
> + desc->rpt_desc.wDescriptorLength);
> if (input_device->report_desc_size == 0) {
> input_device->dev_info_status = -EINVAL;
> goto cleanup;
> @@ -210,7 +210,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
>
> memcpy(input_device->report_desc,
> ((unsigned char *)desc) + desc->bLength,
> - le16_to_cpu(desc->desc[0].wDescriptorLength));
> + le16_to_cpu(desc->rpt_desc.wDescriptorLength));
>
> /* Send the ack */
> memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index a6eb6fe6130d..f8b853180680 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -983,12 +983,11 @@ static int usbhid_parse(struct hid_device *hid)
> struct usb_host_interface *interface = intf->cur_altsetting;
> struct usb_device *dev = interface_to_usbdev (intf);
> struct hid_descriptor *hdesc;
> + struct hid_class_descriptor *hcdesc;
> u32 quirks = 0;
> unsigned int rsize = 0;
> char *rdesc;
> - int ret, n;
> - int num_descriptors;
> - size_t offset = offsetof(struct hid_descriptor, desc);
> + int ret;
>
> quirks = hid_lookup_quirk(hid);
>
> @@ -1010,20 +1009,19 @@ 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");
> + if (!hdesc->bNumDescriptors ||
> + hdesc->bLength != sizeof(*hdesc) +
> + (hdesc->bNumDescriptors - 1) * sizeof(*hcdesc)) {
> + dbg_hid("hid descriptor invalid, bLen=%hhu bNum=%hhu\n",
> + hdesc->bLength, hdesc->bNumDescriptors);
> return -EINVAL;
> }
>
> hid->version = le16_to_cpu(hdesc->bcdHID);
> hid->country = hdesc->bCountryCode;
>
> - num_descriptors = min_t(int, hdesc->bNumDescriptors,
> - (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
> -
> - for (n = 0; n < num_descriptors; n++)
> - if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> - rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> + if (hdesc->rpt_desc.bDescriptorType == HID_DT_REPORT)
> + rsize = le16_to_cpu(hdesc->rpt_desc.wDescriptorLength);
>
> if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> dbg_hid("weird size of report descriptor (%u)\n", rsize);
> @@ -1051,6 +1049,11 @@ static int usbhid_parse(struct hid_device *hid)
> goto err;
> }
>
> + if (hdesc->bNumDescriptors > 1)
> + hid_warn(intf,
> + "%u unsupported optional hid class descriptors\n",
> + (int)(hdesc->bNumDescriptors - 1));
> +
> hid->quirks |= quirks;
>
> return 0;
> --
> 2.43.0
>
For the hid-hyperv.c changes,
Reviewed-by: Michael Kelley <mhklinux@...look.com>
Powered by blists - more mailing lists