On Thu, Feb 25, 2021 at 08:52:15PM +0600, Sabyrzhan Tasbolatov wrote:
> syzbot found WARNING in hid_alloc_report_buf[1] when the raw buffer for
> report is kmalloc() allocated with length > KMALLOC_MAX_SIZE, causing
> order >= MAX_ORDER condition:
>
> 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);
>
> The restriction with HID_MAX_BUFFER_SIZE (16kb) is, seems, a valid max
> limit. I've come up with this in all hid_report_len() xrefs.
>
> The fix inside hid_report_len(), not in *hid_alloc_report_buf() is also
> fixing out-of-bounds here in memcpy():
>
> statc int hid_submit_ctrl(..)
> {
> ..
> len = hid_report_len(report);
> if (dir == USB_DIR_OUT) {
> ..
> if (raw_report) {
> memcpy(usbhid->ctrlbuf, raw_report, len);
> ..
>
> So I've decided to return HID_MAX_BUFFER_SIZE if it the report length is
> bigger than limit, otherwise the return the report length.
>
> [1]
> Call Trace:
> alloc_pages include/linux/gfp.h:547 [inline]
> kmalloc_order+0x40/0x130 mm/slab_common.c:837
> kmalloc_order_trace+0x15/0x70 mm/slab_common.c:853
> kmalloc_large include/linux/slab.h:481 [inline]
> __kmalloc+0x257/0x330 mm/slub.c:3974
> kmalloc include/linux/slab.h:557 [inline]
> hid_alloc_report_buf+0x70/0xa0 drivers/hid/hid-core.c:1648
> __usbhid_submit_report drivers/hid/usbhid/hid-core.c:590 [inline]
>
> Reported-by: syzbot+ab02336a647181a886a6@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@xxxxxxxxx>
> ---
> drivers/hid/usbhid/hid-core.c | 2 +-
> include/linux/hid.h | 4 +++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 86257ce6d619..4e9077363c96 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -374,7 +374,7 @@ static int hid_submit_ctrl(struct hid_device *hid)
> raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report;
> dir = usbhid->ctrl[usbhid->ctrltail].dir;
>
> - len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
> + len = hid_report_len(report);
> if (dir == USB_DIR_OUT) {
> usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
> usbhid->urbctrl->transfer_buffer_length = len;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index c39d71eb1fd0..509a6ffdca00 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -1156,7 +1156,9 @@ static inline void hid_hw_wait(struct hid_device *hdev)
> static inline u32 hid_report_len(struct hid_report *report)
> {
> /* equivalent to DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) */
> - return ((report->size - 1) >> 3) + 1 + (report->id > 0);
> + u32 len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
> +
> + return len > HID_MAX_BUFFER_SIZE ? HID_MAX_BUFFER_SIZE : len;
Won't this cause silent errors?
How about instead just rejecting any device which includes a report
whose length is too big (along with an line in the system log explaining
what's wrong)?
Alan Stern
