linux-kernel - Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in usbhid

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f7963a1d-e069-4ec9-82a1-e17fd324a44f@cosmicgizmosystems.com>
Date: Wed, 29 Jan 2025 11:21:25 -0800
From: Terry Junge <linuxhid@...micgizmosystems.com>
To: Kees Cook <kees@...nel.org>, Alan Stern <stern@...land.harvard.edu>
Cc: Nikita Zhandarovich <n.zhandarovich@...tech.ru>,
 Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>,
 linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
 syzkaller-bugs@...glegroups.com, linux-kernel@...r.kernel.org,
 syzbot+c52569baf0c843f35495@...kaller.appspotmail.com,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in
 usbhid_parse()

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
>