Hi Alan, On Jul 08 2025, Alan Stern wrote: > Jiri and Benjamin: > > Investigating a recent bug report from syzbot > (https://lore.kernel.org/linux-usb/686be237.a70a0220.29fe6c.0b0c.GAE@xxxxxxxxxx/) > led me to a rather serious error in the HID core. It could affect a > lot of drivers, and I don't know enough about them or the HID subsystem > to fix it right away. > > In short, does the value returned by hid_report_len() count the byte > reserved for the report ID number? It depends :) See your analysis below... > > Some drivers seem to assume that it does and some seem to assume that it > doesn't. Here's what the actual code from include/linux/hid.h does: > > /** > * hid_report_len - calculate the report length > * > * @report: the report we want to know the length > */ > static inline u32 hid_report_len(struct hid_report *report) > { > return DIV_ROUND_UP(report->size, 8) + (report->id > 0); > } > > It's somewhere in between -- it includes the ID byte in the count if and > only if the ID is nonzero! And of course, this behavior isn't mentioned > in the (ungrammatical) kerneldoc. Yeah, the docs are bad. But your analysis is correct: for a given report, its size might include or not the report ID. This is because the report ID is optional in the HID spec, and some devices don't make use of it. Those devices are only exporting one report type, and can not have another report type in the same HID device. If a device needs to have more than one, it then needs to export the report ID in all of its reports, and then report_len count that extra byte. To give you an example, old mice (or inexpensive ones), would export a report with: - X, Y, Wheel, B0, B1, B2 When a more advanced one might export: - REPORT ID 0, X, Y, Wheel, B0, B1, B2, ... B9 - REPORT ID 1, SOME_VERY_COMPLEX_CONFIGURATION, ANOTHER ONE, ETC... However, a HID device making use of the non report ID version can never make use of a report ID (or it's custom proprietary protocol). > > The particular scenario causing the bug found by syzbot was this: > report->size was 0, report->id was 0, and the lower-level driver (usbhid > in this case) assumed that the length argument (which was 0) did include > the ID byte. Since the ID was 0, the driver skipped over the first byte > of the buffer and decremented the length, causing the length to > underflow and leading to an invalid memory access. In a more realistic > setting, this would merely result in data loss and leakage. That scenario is bogus (like most of syzbots in the HID space). There should be no way the HID subsystem allows a report->size of 0 to be used in a set_report or a get_report. So that's where the bug lies: HID trusts too much the device, and can lead to that kind of issues. > > How should this be fixed? We have 2 problems here: - and __hid_request or __hid_raw_request should reject a null size report, and in the __hid_request case, a report of size 1 if there is a report ID. - Why is hidinput_change_resolution_multipliers() even considfering this report of size 0 when it should at least ensure that there is one field within the report The first one should be easy to fix: add a bunch of checks. __hid_hw_raw_request() seems to not expose the problem actually. The second one would need a little bit more understanding of the fake report descriptor provided by syzbot. > > Related issue: When the lower-level driver's raw_request() routine is > called, can it assume that the first byte of the output buffer always > contains the report ID number, set by the HID core? If not, should it > assume that the first byte is always reserved for the ID, or that the > first byte is reserved only when the ID is nonzero? The first byte should always be reserved to the report ID, and is populated by 0 by hid-core when the report ID is not in use. So the usbhid driver is doing the right thing. In case there is no report ID, it ignores the first byte when making a request to the device, because in my example above, the device would not expect the first byte to be 0. > > Do __hid_request() and __hid_hw_raw_request() behave the same way in > this regard? __hid_hw_raw_request() ensures that there is a minimum length of one, so we should be fine. __hid_request apparently is the only one giving too much trust in the report. Cheers, Benjamin > > Alan Stern
