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

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(+)

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?

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...

Hello,

On 6/4/24 10:45, Alan Stern wrote:
> 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.

Alan, I finally got around to preparing a revised version of the
required patch and encountered a few issues. I could use some advice in
this matter...

If we change 'struct hid_descriptor' as you suggested, which does make
sense, most occurrences of that type are easy enough to fix.

1) usbhid_parse() starts working properly if there are more than 1
descriptors, sizeof(struct hid_descriptor) may be turned into something
crude but straightforward like sizeof(struct hid_descriptor) +
sizeof(struct hid_class_descriptor).

2) 'hid_descriptor' in drivers/hid/hid-hyperv.c remains innocuous as
well as only 1 descriptor expected there. My impression is only some
small changes are needed there.

However, the issue that stumps me is the following: static struct
hid_descriptor hidg_desc in drivers/usb/gadget/function/f_hid.c relies
on a static nature of that one descriptor. hidg_desc ends up being used
elsewhere, in other static structures. Basically, using __counted_by
requires a lot of changes, as I see it, out of scope of merely closing
an UBSAN error.

Is this approach still worthy pursuing or should I look into some neater
solution?

Best regards,
Nikita

>>>>
>>>> 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

On Tue, Jan 28, 2025 at 05:45:21AM -0800, Nikita Zhandarovich wrote:
> Hello,
> 
> On 6/4/24 10:45, Alan Stern wrote:
> > 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.
> 
> Alan, I finally got around to preparing a revised version of the
> required patch and encountered a few issues. I could use some advice in
> this matter...
> 
> If we change 'struct hid_descriptor' as you suggested,

I didn't make that suggestion.  Kees Cook did.

>  which does make
> sense, most occurrences of that type are easy enough to fix.
> 
> 1) usbhid_parse() starts working properly if there are more than 1
> descriptors, sizeof(struct hid_descriptor) may be turned into something
> crude but straightforward like sizeof(struct hid_descriptor) +
> sizeof(struct hid_class_descriptor).
> 
> 2) 'hid_descriptor' in drivers/hid/hid-hyperv.c remains innocuous as
> well as only 1 descriptor expected there. My impression is only some
> small changes are needed there.
> 
> However, the issue that stumps me is the following: static struct
> hid_descriptor hidg_desc in drivers/usb/gadget/function/f_hid.c relies
> on a static nature of that one descriptor. hidg_desc ends up being used
> elsewhere, in other static structures. Basically, using __counted_by
> requires a lot of changes, as I see it, out of scope of merely closing
> an UBSAN error.

The hidg_desc structure needs to contain room for a single 
hid_descriptor containing a single hid_class_descriptor.  I think you 
can define it that way by doing something like this:

static struct hid_descriptor hidg_desc = {
	.bLength			= sizeof hidg_desc,
	.bDescriptorType		= HID_DT_HID,
	.bcdHID				= cpu_to_le16(0x0101),
	.bCountryCode			= 0x00,
	.bNumDescriptors		= 0x1,
	.desc				= {
		{
			.bDescriptorType	= 0, /* DYNAMIC */
			.wDescriptorLength	= 0, /* DYNAMIC */
		}
	}
};

Or maybe it needs to be:

	.desc				= { {0, 0} } /* DYNAMIC */

I'm not sure what is the correct syntax; you'll have to figure that out.  

You'll have to be more careful about the definition of hidg_desc_copy in 
hidg_setup(), however.  You might want to define hidg_desc_copy as an 
alias to the start of a byte array of the right size.

> Is this approach still worthy pursuing or should I look into some neater
> solution?

I think you should persist with this approach.

Alan Stern

Sorry to join late and top post but I want to propose a direction change for this.

According to the HID 1.11 specification section 6.2.1, the first element of the desc array must be the type and size of the mandatory report descriptor. There is no need to scan through the array to look for it. So the for loop can be collapsed to:

	if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
		rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);

and the out-of-bounds bug goes away.

I would be happy to submit a patch if you like.

Thanks,
Terry

On 1/28/25 5:53 PM, Kees Cook wrote:
> On Tue, Jan 28, 2025 at 12:00:41PM -0500, Alan Stern wrote:
>> On Tue, Jan 28, 2025 at 05:45:21AM -0800, Nikita Zhandarovich wrote:
>>> Hello,
>>>
>>> On 6/4/24 10:45, Alan Stern wrote:
>>>> 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.
>>>
>>> Alan, I finally got around to preparing a revised version of the
>>> required patch and encountered a few issues. I could use some advice in
>>> this matter...
>>>
>>> If we change 'struct hid_descriptor' as you suggested,
>>
>> I didn't make that suggestion.  Kees Cook did.
>>
>>>  which does make
>>> sense, most occurrences of that type are easy enough to fix.
>>>
>>> 1) usbhid_parse() starts working properly if there are more than 1
>>> descriptors, sizeof(struct hid_descriptor) may be turned into something
>>> crude but straightforward like sizeof(struct hid_descriptor) +
>>> sizeof(struct hid_class_descriptor).
>>>
>>> 2) 'hid_descriptor' in drivers/hid/hid-hyperv.c remains innocuous as
>>> well as only 1 descriptor expected there. My impression is only some
>>> small changes are needed there.
>>>
>>> However, the issue that stumps me is the following: static struct
>>> hid_descriptor hidg_desc in drivers/usb/gadget/function/f_hid.c relies
>>> on a static nature of that one descriptor. hidg_desc ends up being used
>>> elsewhere, in other static structures. Basically, using __counted_by
>>> requires a lot of changes, as I see it, out of scope of merely closing
>>> an UBSAN error.
>>
>> The hidg_desc structure needs to contain room for a single 
>> hid_descriptor containing a single hid_class_descriptor.  I think you 
>> can define it that way by doing something like this:
>>
>> static struct hid_descriptor hidg_desc = {
>> 	.bLength			= sizeof hidg_desc,
>> 	.bDescriptorType		= HID_DT_HID,
>> 	.bcdHID				= cpu_to_le16(0x0101),
>> 	.bCountryCode			= 0x00,
>> 	.bNumDescriptors		= 0x1,
>> 	.desc				= {
>> 		{
>> 			.bDescriptorType	= 0, /* DYNAMIC */
>> 			.wDescriptorLength	= 0, /* DYNAMIC */
>> 		}
>> 	}
>> };
>>
>> Or maybe it needs to be:
>>
>> 	.desc				= { {0, 0} } /* DYNAMIC */
>>
>> I'm not sure what is the correct syntax; you'll have to figure that out.  
> 
> Either should work.
> 
>>
>> You'll have to be more careful about the definition of hidg_desc_copy in 
>> hidg_setup(), however.  You might want to define hidg_desc_copy as an 
>> alias to the start of a byte array of the right size.
> 
> For an on-stack fixed-size flex array structure, you can use:
> 
> 	DEFINE_FLEX(struct hid_descriptor, hidg_desc_copy,
> 		    desc, bNumDescriptors, 1);
> 	*hidg_desc_copy = hidg_desc;
> 
> and then adjust the "hidg_desc_copy." instances to "hidg_desc_copy->"
> 
>>
>>> Is this approach still worthy pursuing or should I look into some neater
>>> solution?
>>
>> I think you should persist with this approach.
>>
>> Alan Stern
>