On Tue, Jul 08, 2025 at 05:51:08PM +0200, Benjamin Tissoires wrote: > 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). So you're saying that while the kerneldoc for hid_report_len() could be improved, the code itself is correct. Fine. But this merely means that the real problem lies elsewhere (and _not_ in syzbot's test scenario!). See below. Also, are you sure that all the other HID drivers realize that hid_report_len() behaves this way? It's an easy thing to get wrong. > > 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. I would normally expect this to checked when the report descriptors are read and parsed during probing. A report of length zero could be rejected then (perhaps with an error message in the kernel log). However, that wouldn't fully fix the problem. And besides, length-0 reports are (or should be) harmless. > __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. I suppose we can get the information from syzbot if it's really necessary. But it seems to be a minor point. > > 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. Then why does hid_output_report() do this: if (report->id > 0) *data++ = report->id; ? The first byte is not reserved for the ID when the ID is 0. According to what you said, the assignment should be unconditional. Isn't that a genuine bug? And shouldn't the length computed by hid_alloc_report_buf() be one larger than it is when the ID is 0? > 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. When usbhid skips the first byte, it also decrements the length. Since __hid_request() calls it with a length given by hid_report_len(), the length has _already_ been decremented (or rather, not incremented -- it doesn't account for the ID byte), and consequently the USB transfer will leave out the final byte of the report. This is the data loss I mentioned earlier. It's another bug. So which is doing the wrong thing: usbhid or __hid_request()? > > 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. In principle there's nothing wrong with a zero-length report. It would be pretty useless, but nevertheless we should be able to handle one without crashing the kernel. Alan Stern
