| Message ID | 20240524120112.28076-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse() | expand |
On Fri, 24 May 2024, Nikita Zhandarovich wrote: > 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> Applied, thanks.
On June 4, 2024 1:15:35 AM PDT, Jiri Kosina <jikos@kernel.org> wrote: >On Fri, 24 May 2024, Nikita Zhandarovich wrote: > >> 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> > >Applied, thanks. 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.
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?
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. Regards, Nikita
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...
On Tue, Jun 04, 2024 at 10:21:15AM -0700, Kees Cook wrote: > 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. That's a lower limit, not an upper limit. The hid_descriptor must include at least one hid_class_descriptor, but it can include more. That's what the min_t() calculation of num_descriptors is meant to figure out. > Is more than 1 descriptor expected to > work correctly? More than one hid_class_descriptor -- yes. > Or is the limit being ignored? I'm a bit confused by > this code... Does this explain it? Alan Stern
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);
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(+)