| 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: <20250131151600.410242-1-n.zhandarovich@fintech.ru>
Date: Fri, 31 Jan 2025 18:15:58 +0300
From: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
To: Jiri Kosina <jikos@...nel.org>, Alan Stern <stern@...land.harvard.edu>,
Kees Cook <kees@...nel.org>
CC: Nikita Zhandarovich <n.zhandarovich@...tech.ru>, 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: [PATCH v2] HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse()
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;
}
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) {
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) \
Powered by blists - more mailing lists