Thread (4 messages) 4 messages, 2 authors, 2021-02-26

Re: [PATCH] drivers/hid: fix for the big hid report length

From: Alan Stern <stern@rowland.harvard.edu>
Date: 2021-02-25 16:00:01
Also in: linux-usb, lkml

On Thu, Feb 25, 2021 at 08:52:15PM +0600, Sabyrzhan Tasbolatov wrote:
quoted hunk ↗ jump to hunk
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@syzkaller.appspotmail.com
Signed-off-by: Sabyrzhan Tasbolatov <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help