| 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: <202501311205.DB75F95@keescook>
Date: Fri, 31 Jan 2025 12:12:53 -0800
From: Kees Cook <kees@...nel.org>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
Cc: Jiri Kosina <jikos@...nel.org>, Alan Stern <stern@...land.harvard.edu>,
Benjamin Tissoires <bentiss@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Terry Junge <linuxhid@...micgizmosystems.com>,
linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
syzbot <syzbot+c52569baf0c843f35495@...kaller.appspotmail.com>,
syzkaller-bugs@...glegroups.com, lvc-project@...uxtesting.org
Subject: Re: [PATCH v2] HID: usbhid: fix recurrent out-of-bounds bug in
usbhid_parse()
On Fri, Jan 31, 2025 at 06:15:58PM +0300, Nikita Zhandarovich wrote:
> Syzbot reports [1] a reemerging out-of-bounds bug regarding hid
> descriptors supposedly having unpredictable bNumDescriptors values in
> usbhid_parse().
>
> The issue stems from the fact that hid_class_descriptor is supposed
> to be a flexible array, however it was sized as desc[1], using only
> one element. Therefore, going beyond 1 element, courtesy of
> bNumDescriptors, may lead to an error.
>
> Modify struct hid_descriptor by employing __counted_by macro, tying
> together struct hid_class_descriptor desc[] and number of descriptors
> bNumDescriptors. Also, fix places where this change affects work with
> newly updated struct.
>
> [1] Syzbot report:
>
> UBSAN: array-index-out-of-bounds in drivers/hid/usbhid/hid-core.c:1024:7
> index 1 is out of range for type 'struct hid_class_descriptor[1]'
> ...
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0x241/0x360 lib/dump_stack.c:114
> ubsan_epilogue lib/ubsan.c:231 [inline]
> __ubsan_handle_out_of_bounds+0x121/0x150 lib/ubsan.c:429
> usbhid_parse+0x5a7/0xc80 drivers/hid/usbhid/hid-core.c:1024
> hid_add_device+0x132/0x520 drivers/hid/hid-core.c:2790
> usbhid_probe+0xb38/0xea0 drivers/hid/usbhid/hid-core.c:1429
> usb_probe_interface+0x645/0xbb0 drivers/usb/core/driver.c:399
> really_probe+0x2b8/0xad0 drivers/base/dd.c:656
> __driver_probe_device+0x1a2/0x390 drivers/base/dd.c:798
> driver_probe_device+0x50/0x430 drivers/base/dd.c:828
> __device_attach_driver+0x2d6/0x530 drivers/base/dd.c:956
> bus_for_each_drv+0x24e/0x2e0 drivers/base/bus.c:457
> __device_attach+0x333/0x520 drivers/base/dd.c:1028
> bus_probe_device+0x189/0x260 drivers/base/bus.c:532
> device_add+0x8ff/0xca0 drivers/base/core.c:3720
> usb_set_configuration+0x1976/0x1fb0 drivers/usb/core/message.c:2210
> usb_generic_driver_probe+0x88/0x140 drivers/usb/core/generic.c:254
> usb_probe_device+0x1b8/0x380 drivers/usb/core/driver.c:294
>
> 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: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
> v1: https://lore.kernel.org/all/20240524120112.28076-1-n.zhandarovich@fintech.ru/
>
> v2: Instead of essentially forcing usbhid_parse() to only check
> the first descriptor, modify hid_descriptor struct to anticipate
> multiple hid_class_descriptor in desc[] by utilizing __counted_by
> macro. This change, in turn, requires several other ones wherever
> that struct is used. Adjust commit description accordingly.
>
> P.S. Since syzbot currently breaks trying to reproduce the issue,
> with or without this patch, I only managed to test it locally with
> syz repros. Would greatly appreciate other people's effort to test
> it as well.
>
> P.P.S. Terry Junge <linuxhid@...micgizmosystems.com> suggested a
> different approach to this issue, see:
> Link: https://lore.kernel.org/all/f7963a1d-e069-4ec9-82a1-e17fd324a44f@cosmicgizmosystems.com/
>
> drivers/hid/usbhid/hid-core.c | 2 +-
> drivers/usb/gadget/function/f_fs.c | 3 ++-
> drivers/usb/gadget/function/f_hid.c | 22 ++++++++++++++--------
> include/linux/hid.h | 2 +-
> 4 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index a6eb6fe6130d..eb4807785d6d 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1010,7 +1010,7 @@ static int usbhid_parse(struct hid_device *hid)
> return -ENODEV;
> }
>
> - if (hdesc->bLength < sizeof(struct hid_descriptor)) {
> + if (hdesc->bLength < struct_size(hdesc, desc, hdesc->bNumDescriptors)) {
> dbg_hid("hid descriptor is too short\n");
> return -EINVAL;
> }
I don't think you want this hunk in the patch. The existing logic will
correctly adapt num_descriptors to a size that fits within
hdesc->bLength. In theory, the above change could break a weird but
working device that reported too high bNumDescriptors but still worked
with what num_descriptors walks.
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 2dea9e42a0f8..a4b6d7cbf56d 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2550,7 +2550,8 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
> case USB_TYPE_CLASS | 0x01:
> if (*current_class == USB_INTERFACE_CLASS_HID) {
> pr_vdebug("hid descriptor\n");
> - if (length != sizeof(struct hid_descriptor))
> + if (length < sizeof(struct hid_descriptor) +
> + sizeof(struct hid_class_descriptor))
> goto inv_length;
> break;
> } else if (*current_class == USB_INTERFACE_CLASS_CCID) {
Same here, I think? This isn't needed unless I'm misunderstanding
something about the fix.
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 740311c4fa24..ec8c2e2d6812 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -139,13 +139,17 @@ static struct usb_interface_descriptor hidg_interface_desc = {
> };
>
> static struct hid_descriptor hidg_desc = {
> - .bLength = sizeof hidg_desc,
> + .bLength = struct_size(&hidg_desc, desc, 1),
> .bDescriptorType = HID_DT_HID,
> .bcdHID = cpu_to_le16(0x0101),
> .bCountryCode = 0x00,
> .bNumDescriptors = 0x1,
> - /*.desc[0].bDescriptorType = DYNAMIC */
> - /*.desc[0].wDescriptorLenght = DYNAMIC */
> + .desc = {
> + {
> + .bDescriptorType = 0, /* DYNAMIC */
> + .wDescriptorLength = 0, /* DYNAMIC */
> + }
> + }
> };
>
> /* Super-Speed Support */
> @@ -936,16 +940,18 @@ static int hidg_setup(struct usb_function *f,
> switch (value >> 8) {
> case HID_DT_HID:
> {
> - struct hid_descriptor hidg_desc_copy = hidg_desc;
> + DEFINE_FLEX(struct hid_descriptor, hidg_desc_copy,
> + desc, bNumDescriptors, 1);
> + *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->desc[0].bDescriptorType = HID_DT_REPORT;
> + hidg_desc_copy->desc[0].wDescriptorLength =
> cpu_to_le16(hidg->report_desc_length);
>
> length = min_t(unsigned short, length,
> - hidg_desc_copy.bLength);
> - memcpy(req->buf, &hidg_desc_copy, length);
> + hidg_desc_copy->bLength);
> + memcpy(req->buf, hidg_desc_copy, length);
> goto respond;
> break;
> }
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cdc0dc13c87f..85a58ae2c4a0 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -739,7 +739,7 @@ struct hid_descriptor {
> __u8 bCountryCode;
> __u8 bNumDescriptors;
>
> - struct hid_class_descriptor desc[1];
> + struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
> } __attribute__ ((packed));
>
> #define HID_DEVICE(b, g, ven, prod) \
Otherwise, this looks correct to me.
--
Kees Cook
Powered by blists - more mailing lists