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? 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. 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. How should this be fixed? 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? Do __hid_request() and __hid_hw_raw_request() behave the same way in this regard? Alan Stern
