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
