Re: [linux-6.6.y 2/3] HID: pass the buffer size to hid_report_raw_event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The patch fails to resolve the short report regression because `hid_input_report()` incorrectly defaults `bufsize` to the received payload `size`.
- [High] `wacom_wac_queue_flush()` incorrectly passes `size` instead of `sizeof(buf)` as the `bufsize` parameter.
- [Low] Incorrect format specifiers in `hid_warn_ratelimited` cause compiler warnings on 32-bit platforms.

Pre-existing issues:
- [Critical] Unbounded `memcpy` in `dispatch_hid_bpf_device_event` leads to a heap buffer overflow.
--

commit e280aff3473f90b2372da480e1f061c96bb64fd0
Author: Benjamin Tissoires <bentiss@kernel.org>

HID: pass the buffer size to hid_report_raw_event

This patch adds a buffer size parameter to hid_report_raw_event() to better
differentiate between the allocated buffer capacity and the actual payload
size. This aims to reject buffer overflow attempts while safely zero-padding
valid short reports. The changes propagate the new parameter through various
HID transport drivers.

[ Upstream commit 2c85c61d1332e1e16f020d76951baf167dcb6f7a ]

> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 7903c8638e817..e0c03942ff7c4 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -47,7 +47,7 @@ __weak noinline int hid_bpf_device_event(struct hid_bpf_ctx *ctx)
>  
>  u8 *
>  dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data,
> -			      u32 *size, int interrupt)
> +			      size_t *buf_size, u32 *size, int interrupt)
>  {

[Severity: Critical]
This isn't a bug introduced by this patch, but can the memcpy inside this
function cause a heap buffer overflow?

When dispatch_hid_bpf_device_event() handles incoming events, it executes:

    memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
    memcpy(ctx_kern.data, data, *size);

If a malicious device or a compromised userspace process using /dev/uhid
injects a report with a payload up to 4096 bytes (UHID_DATA_MAX), and this
exceeds hdev->bpf.allocated_data, doesn't this unbounded memcpy overwrite
the heap?

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index e3d728d67b538..c6d98634b97e6 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
[ ... ]
> @@ -1986,24 +1986,32 @@ int __hid_request(struct hid_device *hid, struct hid_report *report,
>  }
>  EXPORT_SYMBOL_GPL(__hid_request);
>  
> -int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size,
> -			 int interrupt)
> +int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data,
> +			 size_t bufsize, u32 size, int interrupt)
>  {
>  	struct hid_report_enum *report_enum = hid->report_enum + type;
>  	struct hid_report *report;
>  	struct hid_driver *hdrv;
>  	int max_buffer_size = HID_MAX_BUFFER_SIZE;
>  	u32 rsize, csize = size;
> +	size_t bsize = bufsize;
>  	u8 *cdata = data;
>  	int ret = 0;
>  
>  	report = hid_get_report(report_enum, data);
>  	if (!report)
> -		goto out;
> +		return 0;
> +
> +	if (unlikely(bsize < csize)) {
> +		hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n",
> +				     report->id, csize, bsize);

[Severity: Low]
Does using %ld for size_t cause compiler warnings on 32-bit platforms?

Since size_t is an unsigned int on 32-bit architectures, shouldn't we use
%zu for bsize, and %u instead of %d for csize?

> +		return -EINVAL;
> +	}
>  
[ ... ]
> @@ -2055,12 +2069,13 @@ EXPORT_SYMBOL_GPL(hid_report_raw_event);
>   *
>   * This is data entry for lower layers.
>   */
> -int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size,
> -		     int interrupt)
> +int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data,
> +		     u32 size, int interrupt)
>  {
>  	struct hid_report_enum *report_enum;
>  	struct hid_driver *hdrv;
>  	struct hid_report *report;
> +	size_t bufsize = size;

[Severity: High]
Does setting bufsize to size defeat the purpose of the new bounds checks?

By initializing bufsize to the received payload size instead of the
actual buffer capacity, the new checks in hid_report_raw_event():

    if (bsize < rsize) {
        hid_warn_ratelimited(...);
        return -EINVAL;
    }

will always evaluate to true for short reports, as bsize will equal the short
payload size (csize). Will this unconditionally reject short reports and
leave the original regression unfixed for standard HID devices?

>  	int ret = 0;
>  
[ ... ]
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 52011503ff3ba..52b7f474d8669 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -74,7 +74,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
>  		int err;
>  
>  		size = kfifo_out(fifo, buf, sizeof(buf));
> -		err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, false);
> +		err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, size, false);

[Severity: High]
Should this pass sizeof(buf) as the buffer size instead of size?

Because buf is allocated as u8 buf[WACOM_PKGLEN_MAX] in this function,
passing size (the number of bytes read) as both bufsize and size means
the buffer size limit equals the payload size.

If this is a valid short report, won't it be incorrectly rejected by the
new size checks in hid_report_raw_event()?

>  		if (err) {
>  			hid_warn(hdev, "%s: unable to flush event due to error %d\n",
>  				 __func__, err);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603133140.3069226-2-lee@kernel.org?part=1