On Wed, 11 Dec 2019, Alan Stern wrote:
> > > The syzbot fuzzer found a slab-out-of-bounds bug in the HID report
> > > handler. The bug was caused by a report descriptor which included a
> > > field with size 12 bits and count 4899, for a total size of 7349
> > > bytes.
> > >
> > > The usbhid driver uses at most a single-page 4-KB buffer for reports.
> > > In the test there wasn't any problem about overflowing the buffer,
> > > since only one byte was received from the device. Rather, the bug
> > > occurred when the HID core tried to extract the data from the report
> > > fields, which caused it to try reading data beyond the end of the
> > > allocated buffer.
> > >
> > > This patch fixes the problem by rejecting any report whose total
> > > length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> > > for a possible report index). In theory a device could have a report
> > > longer than that, but if there was such a thing we wouldn't handle it
> > > correctly anyway.
> > >
> > > Reported-and-tested-by: syzbot+09ef48aa58261464b621@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > CC: <stable@xxxxxxxxxxxxxxx>
> >
> > Thanks for hunting this down Alan. Applied.
>
> I just noticed this code:
>
> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
> {
> /*
> * 7 extra bytes are necessary to achieve proper functionality
> * of implement() working on 8 byte chunks
> */
>
> u32 len = hid_report_len(report) + 7;
>
> return kmalloc(len, flags);
> }
>
> Does this indicate that the upper limit on a report length should
> really be HID_MAX_BUFFER_SIZE - 8 instead of HID_MAX_BUFFER_SIZE - 1?
As far as I remember, this is just very lousy way of properly rounding the
size up (see 27ce405039bfe). So I believe HID_MAX_BUFFER_SIZE -1 is still
functionally correct.
Thanks,
--
Jiri Kosina
SUSE Labs
