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
