On Fri, Jan 31, 2025 at 06:15:58PM +0300, Nikita Zhandarovich wrote: > Syzbot reports [1] a reemerging out-of-bounds bug regarding hid > descriptors supposedly having unpredictable bNumDescriptors values in > usbhid_parse(). > > The issue stems from the fact that hid_class_descriptor is supposed > to be a flexible array, however it was sized as desc[1], using only > one element. Therefore, going beyond 1 element, courtesy of > bNumDescriptors, may lead to an error. > > Modify struct hid_descriptor by employing __counted_by macro, tying > together struct hid_class_descriptor desc[] and number of descriptors > bNumDescriptors. Also, fix places where this change affects work with > newly updated struct. > > [1] Syzbot report: > > 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]' > ... > 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-by: syzbot+c52569baf0c843f35495@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=c52569baf0c843f35495 > Fixes: f043bfc98c19 ("HID: usbhid: fix out-of-bounds bug") > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> > --- > v1: https://lore.kernel.org/all/20240524120112.28076-1-n.zhandarovich@xxxxxxxxxx/ > > v2: Instead of essentially forcing usbhid_parse() to only check > the first descriptor, modify hid_descriptor struct to anticipate > multiple hid_class_descriptor in desc[] by utilizing __counted_by > macro. This change, in turn, requires several other ones wherever > that struct is used. Adjust commit description accordingly. > > P.S. Since syzbot currently breaks trying to reproduce the issue, > with or without this patch, I only managed to test it locally with > syz repros. Would greatly appreciate other people's effort to test > it as well. > > P.P.S. Terry Junge <linuxhid@xxxxxxxxxxxxxxxxxxxxxx> suggested a > different approach to this issue, see: > Link: https://lore.kernel.org/all/f7963a1d-e069-4ec9-82a1-e17fd324a44f@xxxxxxxxxxxxxxxxxxxxxx/ > > drivers/hid/usbhid/hid-core.c | 2 +- > drivers/usb/gadget/function/f_fs.c | 3 ++- > drivers/usb/gadget/function/f_hid.c | 22 ++++++++++++++-------- > include/linux/hid.h | 2 +- > 4 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > index a6eb6fe6130d..eb4807785d6d 100644 > --- a/drivers/hid/usbhid/hid-core.c > +++ b/drivers/hid/usbhid/hid-core.c > @@ -1010,7 +1010,7 @@ static int usbhid_parse(struct hid_device *hid) > return -ENODEV; > } > > - if (hdesc->bLength < sizeof(struct hid_descriptor)) { > + if (hdesc->bLength < struct_size(hdesc, desc, hdesc->bNumDescriptors)) { > dbg_hid("hid descriptor is too short\n"); > return -EINVAL; > } I don't think you want this hunk in the patch. The existing logic will correctly adapt num_descriptors to a size that fits within hdesc->bLength. In theory, the above change could break a weird but working device that reported too high bNumDescriptors but still worked with what num_descriptors walks. > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 2dea9e42a0f8..a4b6d7cbf56d 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -2550,7 +2550,8 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len, > case USB_TYPE_CLASS | 0x01: > if (*current_class == USB_INTERFACE_CLASS_HID) { > pr_vdebug("hid descriptor\n"); > - if (length != sizeof(struct hid_descriptor)) > + if (length < sizeof(struct hid_descriptor) + > + sizeof(struct hid_class_descriptor)) > goto inv_length; > break; > } else if (*current_class == USB_INTERFACE_CLASS_CCID) { Same here, I think? This isn't needed unless I'm misunderstanding something about the fix. > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > index 740311c4fa24..ec8c2e2d6812 100644 > --- a/drivers/usb/gadget/function/f_hid.c > +++ b/drivers/usb/gadget/function/f_hid.c > @@ -139,13 +139,17 @@ static struct usb_interface_descriptor hidg_interface_desc = { > }; > > static struct hid_descriptor hidg_desc = { > - .bLength = sizeof hidg_desc, > + .bLength = struct_size(&hidg_desc, desc, 1), > .bDescriptorType = HID_DT_HID, > .bcdHID = cpu_to_le16(0x0101), > .bCountryCode = 0x00, > .bNumDescriptors = 0x1, > - /*.desc[0].bDescriptorType = DYNAMIC */ > - /*.desc[0].wDescriptorLenght = DYNAMIC */ > + .desc = { > + { > + .bDescriptorType = 0, /* DYNAMIC */ > + .wDescriptorLength = 0, /* DYNAMIC */ > + } > + } > }; > > /* Super-Speed Support */ > @@ -936,16 +940,18 @@ static int hidg_setup(struct usb_function *f, > switch (value >> 8) { > case HID_DT_HID: > { > - struct hid_descriptor hidg_desc_copy = hidg_desc; > + DEFINE_FLEX(struct hid_descriptor, hidg_desc_copy, > + desc, bNumDescriptors, 1); > + *hidg_desc_copy = hidg_desc; > > VDBG(cdev, "USB_REQ_GET_DESCRIPTOR: HID\n"); > - hidg_desc_copy.desc[0].bDescriptorType = HID_DT_REPORT; > - hidg_desc_copy.desc[0].wDescriptorLength = > + hidg_desc_copy->desc[0].bDescriptorType = HID_DT_REPORT; > + hidg_desc_copy->desc[0].wDescriptorLength = > cpu_to_le16(hidg->report_desc_length); > > length = min_t(unsigned short, length, > - hidg_desc_copy.bLength); > - memcpy(req->buf, &hidg_desc_copy, length); > + hidg_desc_copy->bLength); > + memcpy(req->buf, hidg_desc_copy, length); > goto respond; > break; > } > diff --git a/include/linux/hid.h b/include/linux/hid.h > index cdc0dc13c87f..85a58ae2c4a0 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -739,7 +739,7 @@ struct hid_descriptor { > __u8 bCountryCode; > __u8 bNumDescriptors; > > - struct hid_class_descriptor desc[1]; > + struct hid_class_descriptor desc[] __counted_by(bNumDescriptors); > } __attribute__ ((packed)); > > #define HID_DEVICE(b, g, ven, prod) \ Otherwise, this looks correct to me. -- Kees Cook
