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: [email protected]
Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
drivers/hid/usbhid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
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);
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: [email protected]
> Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
On June 4, 2024 1:15:35 AM PDT, Jiri Kosina <[email protected]> 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: [email protected]
>> Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug")
>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>
>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.
--
Kees Cook
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?
--
Jiri Kosina
SUSE Labs
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...
--
Kees Cook
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