On Jul 19 2025, Benjamin Tissoires wrote: > On Jul 17 2025, Alan Stern wrote: > > Testing by the syzbot fuzzer showed that the HID core gets a > > shift-out-of-bounds exception when it tries to convert a 32-bit > > quantity to a 0-bit quantity. This is hardly an unexpected result, > > but it means that we should not accept report fields that have a size > > of zero bits. Similarly, there's no reason to accept report fields > > with a count of zero; either type of item is completely meaningless > > since no information can be conveyed in zero bits. > > > > Reject fields with a size or count of zero, and reject reports > > containing such fields. > > > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Reported-by: syzbot+b63d677d63bcac06cf90@xxxxxxxxxxxxxxxxxxxxxxxxx > > Closes: https://lore.kernel.org/linux-usb/68753a08.050a0220.33d347.0008.GAE@xxxxxxxxxx/ > > Tested-by: syzbot+b63d677d63bcac06cf90@xxxxxxxxxxxxxxxxxxxxxxxxx > > Fixes: dde5845a529f ("[PATCH] Generic HID layer - code split") > > Cc: stable@xxxxxxxxxxxxxxx > > > > --- > > > > The commit listed in the Fixes tag is not really the right one. But > > code motion made tracking it back any further more difficult than I > > wanted to deal with, so I stopped there. That commit is from 2006, > > which is already far enough in the past. > > > > drivers/hid/hid-core.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > Index: usb-devel/drivers/hid/hid-core.c > > =================================================================== > > --- usb-devel.orig/drivers/hid/hid-core.c > > +++ usb-devel/drivers/hid/hid-core.c > > @@ -313,7 +313,14 @@ static int hid_add_field(struct hid_pars > > } > > > > offset = report->size; > > - report->size += parser->global.report_size * parser->global.report_count; > > + i = parser->global.report_size * parser->global.report_count; > > + if (i == 0) { > > + dbg_hid("invalid field size/count 0x%x 0x%x\n", > > + parser->global.report_size, > > + parser->global.report_count); > > + return -1; > > + } > > + report->size += i; > > > > if (parser->device->ll_driver->max_buffer_size) > > max_buffer_size = parser->device->ll_driver->max_buffer_size; > > @@ -464,7 +471,8 @@ static int hid_parser_global(struct hid_ > > > > case HID_GLOBAL_ITEM_TAG_REPORT_SIZE: > > parser->global.report_size = item_udata(item); > > - if (parser->global.report_size > 256) { > > + if (parser->global.report_size > 256 || > > + parser->global.report_size == 0) { > > hid_err(parser->device, "invalid report_size %d\n", > > parser->global.report_size); > > return -1; > > Sigh... I applied this one too quickly before going on holidays. > > This breaks the hid testsuite: > https://gitlab.freedesktop.org/bentiss/hid/-/jobs/80805946 > > (yes, I should have run it before applying). > > So basically, there are devices out there with a "broken" report size of > 0, and this patch now entirely disables them. > > That Saitek gamepad has the following (see tools/testing/selftests/hid/tests/test_gamepad.py): > 0x95, 0x01, # ..Report Count (1) 60 > 0x75, 0x00, # ..Report Size (0) 62 > 0x81, 0x03, # ..Input (Cnst,Var,Abs) 64 > > So we'd need to disable the field, but not invalidate the entire report. > > I'm glad I scheduled this one for the next cycle. > > I'll try to get something next week. > In case you are interested in fixing this, a quick reproducer can be done through virtme-ng: $> vng --verbose --kconfig --skip-module --config tools/testing/selftests/hid/config $> make -j8 $> vng -v --user root --exec "mount bpffs -t bpf /sys/fs/bpf ; pytest ./tools/testing/selftests/hid/tests/test_gamepad.py -v -x --color=yes -k Saitek" Cheers, Benjamin
