Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in usbhid_parse()

[Kees Cook <kees@kernel.org>]

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

-- 
Kees Cook