diff mbox series

HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse()

Message ID 20240524120112.28076-1-n.zhandarovich@fintech.ru
State New
Headers show
Series HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse() | expand

Commit Message

Nikita Zhandarovich May 24, 2024, 12:01 p.m. UTC
Syzbot reports [1] a reemerging out-of-bounds bug regarding hid
descriptors possibly having incorrect bNumDescriptors values in
usbhid_parse().

Build on the previous fix in "HID: usbhid: fix out-of-bounds bug"
and run a sanity-check ensuring that number of descriptors doesn't
exceed the size of desc[] in struct hid_descriptor.

[1] Syzbot report:
Link: https://syzkaller.appspot.com/bug?extid=c52569baf0c843f35495

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]'
CPU: 0 PID: 8 Comm: kworker/0:1 Not tainted 6.9.0-rc6-syzkaller-00290-gb9158815de52 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/27/2024
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-and-tested-by: syzbot+c52569baf0c843f35495@syzkaller.appspotmail.com
Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/hid/usbhid/hid-core.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jiri Kosina June 4, 2024, 2:15 p.m. UTC | #1
On Tue, 4 Jun 2024, Kees Cook wrote:

> This isn't the right solution. The problem is that hid_class_descriptor 
> is a flexible array but was sized as a single element fake flexible 
> array:
> 
> struct hid_descriptor {
> 	   __u8  bLength;
> 	   __u8  bDescriptorType;
> 	   __le16 bcdHID;
> 	   __u8  bCountryCode;
> 	   __u8  bNumDescriptors;
> 
> 	   struct hid_class_descriptor desc[1];
> } __attribute__ ((packed));
> 
> This likely needs to be: 
> 
> struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
> 
> And then check for any sizeof() uses of the struct that might have changed.

Ah, you are of course right, not sure what I was thinking. Thanks a lot 
for catching my brainfart.

I am dropping the patch for now; Nikita, will you please send a refreshed 
one?
Kees Cook June 4, 2024, 5:21 p.m. UTC | #2
On Tue, Jun 04, 2024 at 10:09:43AM -0700, Nikita Zhandarovich wrote:
> Hi,
> 
> On 6/4/24 07:15, Jiri Kosina wrote:
> > On Tue, 4 Jun 2024, Kees Cook wrote:
> > 
> >> This isn't the right solution. The problem is that hid_class_descriptor 
> >> is a flexible array but was sized as a single element fake flexible 
> >> array:
> >>
> >> struct hid_descriptor {
> >> 	   __u8  bLength;
> >> 	   __u8  bDescriptorType;
> >> 	   __le16 bcdHID;
> >> 	   __u8  bCountryCode;
> >> 	   __u8  bNumDescriptors;
> >>
> >> 	   struct hid_class_descriptor desc[1];
> >> } __attribute__ ((packed));
> >>
> >> This likely needs to be: 
> >>
> >> struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
> >>
> >> And then check for any sizeof() uses of the struct that might have changed.
> > 
> > Ah, you are of course right, not sure what I was thinking. Thanks a lot 
> > for catching my brainfart.
> > 
> > I am dropping the patch for now; Nikita, will you please send a refreshed 
> > one?
> > 
> 
> Thanks for catching my mistake.
> 
> I'll gladly send a revised version, hoping to do it very soon.

I spent a little more time looking at this, and I'm not sure I
understand where the actual space for the descriptors comes from?
There's interface->extra that is being parsed, and effectively
hid_descriptor is being mapped into it, but it uses "sizeof(struct
hid_descriptor)" for the limit. Is more than 1 descriptor expected to
work correctly? Or is the limit being ignored? I'm a bit confused by
this code...
diff mbox series

Patch

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index a90ed2ceae84..f38a4bd3a20e 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1020,6 +1020,9 @@  static int usbhid_parse(struct hid_device *hid)
 	num_descriptors = min_t(int, hdesc->bNumDescriptors,
 	       (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
 
+	if (num_descriptors > ARRAY_SIZE(hdesc->desc))
+		num_descriptors = ARRAY_SIZE(hdesc->desc);
+
 	for (n = 0; n < num_descriptors; n++)
 		if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
 			rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);