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

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

[Sabyrzhan Tasbolatov]

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 <snovitoll@gmail.com>
---
 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;
 }
 
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
-- 
2.25.1

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

[Alan Stern]

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@syzkaller.appspotmail.com
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
>  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

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

[Sabyrzhan Tasbolatov]

On Thu, 25 Feb 2021 10:59:14 -0500, Alan Stern wrote:
> Won't this cause silent errors?

Agree. But there are already such as cases like in:

// net/bluetooth/hidp/core.c
static void hidp_process_report(..)
{
	..
	if (len > HID_MAX_BUFFER_SIZE)
		len = HID_MAX_BUFFER_SIZE;
	..

// drivers/hid/hid-core.c
int hid_report_raw_event(..)
{
	..
	rsize = hid_compute_report_size(report);

	if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE)
		rsize = HID_MAX_BUFFER_SIZE - 1;
	else if (rsize > HID_MAX_BUFFER_SIZE)
		rsize = HID_MAX_BUFFER_SIZE;
	..

// drivers/staging/greybus/hid.c
static int gb_hid_start(..)
{
	..
	if (bufsize > HID_MAX_BUFFER_SIZE)
		bufsize = HID_MAX_BUFFER_SIZE;
	..

> 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)?

Not everywhere, but there are already such as logs when > HID_MAX_BUFFER_SIZE

// drivers/hid/hidraw.c
static ssize_t hidraw_send_report(..)
{
	..
	if (count > HID_MAX_BUFFER_SIZE) {
		hid_warn(dev, "pid %d passed too large report\n",
			 task_pid_nr(current));
		ret = -EINVAL;
		goto out;
	}


I've just noticed that hid_compute_report_size() doing the same thing as
hid_report_len(). So I will replace it with latter one with length check.

So in [PATCH v2] I will do following:

 1. replace hid_compute_report_size() with hid_report_len()

 2. in hid_report_len() we can hid_warn() if length > HID_MAX_BUFFER_SIZE,
and return HID_MAX_BUFFER_SIZE. Or we can return 0 in hid_report_len() to let
functions like hid_hw_raw_request(), hid_hw_output_report() to validate
invalid report length and return -EINVAL. Though I'll need to add !length
missing checks in other places.

Please let me know what it's preferred way in 2nd step.

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

[Alan Stern]

On Fri, Feb 26, 2021 at 02:13:36PM +0600, Sabyrzhan Tasbolatov wrote:
> On Thu, 25 Feb 2021 10:59:14 -0500, Alan Stern wrote:
> > Won't this cause silent errors?
> 
> Agree. But there are already such as cases like in:
> 
> // net/bluetooth/hidp/core.c
> static void hidp_process_report(..)
> {
> 	..
> 	if (len > HID_MAX_BUFFER_SIZE)
> 		len = HID_MAX_BUFFER_SIZE;
> 	..
> 
> // drivers/hid/hid-core.c
> int hid_report_raw_event(..)
> {
> 	..
> 	rsize = hid_compute_report_size(report);
> 
> 	if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE)
> 		rsize = HID_MAX_BUFFER_SIZE - 1;
> 	else if (rsize > HID_MAX_BUFFER_SIZE)
> 		rsize = HID_MAX_BUFFER_SIZE;
> 	..
> 
> // drivers/staging/greybus/hid.c
> static int gb_hid_start(..)
> {
> 	..
> 	if (bufsize > HID_MAX_BUFFER_SIZE)
> 		bufsize = HID_MAX_BUFFER_SIZE;
> 	..
> 
> > 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)?
> 
> Not everywhere, but there are already such as logs when > HID_MAX_BUFFER_SIZE
> 
> // drivers/hid/hidraw.c
> static ssize_t hidraw_send_report(..)
> {
> 	..
> 	if (count > HID_MAX_BUFFER_SIZE) {
> 		hid_warn(dev, "pid %d passed too large report\n",
> 			 task_pid_nr(current));
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 
> 
> I've just noticed that hid_compute_report_size() doing the same thing as
> hid_report_len(). So I will replace it with latter one with length check.
> 
> So in [PATCH v2] I will do following:
> 
>  1. replace hid_compute_report_size() with hid_report_len()
> 
>  2. in hid_report_len() we can hid_warn() if length > HID_MAX_BUFFER_SIZE,
> and return HID_MAX_BUFFER_SIZE. Or we can return 0 in hid_report_len() to let
> functions like hid_hw_raw_request(), hid_hw_output_report() to validate
> invalid report length and return -EINVAL. Though I'll need to add !length
> missing checks in other places.
> 
> Please let me know what it's preferred way in 2nd step.

It's been too long since I worked on this stuff; you should check with 
the maintainers.

Another thing to consider: There probably are devices with multiple 
reports, where one of the reports is too big but people only want to use 
the other, smaller reports.  For situations like that, we don't want to 
reject the entire device.

I don't know if parsing a partiall part is a good thing to do.

Alan Stern