Re: [linux-5.15.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 3 potential issue(s) to consider:
- [High] The patch fails to resolve the short report rejection regression for most HID devices because `hid_input_report()` unconditionally sets `bufsize = size`.
- [High] Incorrect `bufsize` argument in `wacom_wac_queue_flush` defeats the mitigation for short reports.
- [Low] Format string mismatch for `size_t` variable `bsize` causes compiler warnings and potential build failures on 32-bit platforms.
--

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

HID: pass the buffer size to hid_report_raw_event

This commit passes the buffer size along with the data size to
hid_report_raw_event() to make a better decision on whether to reject the
buffer or safely memset it to 0 and pass it to the rest of the stack.

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 4fb573ee31b2c..a8d4673c7b8e1 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1775,8 +1775,8 @@ 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, int type, u8 *data, u32 size,
> -		int interrupt)
> +int hid_report_raw_event(struct hid_device *hid, int type, u8 *data,
> +			 size_t bufsize, u32 size, int interrupt)
>  {
>  	struct hid_report_enum *report_enum = hid->report_enum + type;
>  	struct hid_report *report;
> @@ -1784,16 +1784,24 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
>  	int max_buffer_size = HID_MAX_BUFFER_SIZE;
>  	unsigned int a;
>  	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 this cause a compiler warning on 32-bit platforms? bsize is a size_t,
but is being printed with the %ld format specifier, which might trigger a
-Wformat warning since size_t is usually an unsigned int on those
architectures. Should this use %zu instead?

> +		return -EINVAL;
> +	}
>  
>  	if (report_enum->numbered) {
>  		cdata++;
>  		csize--;
> +		bsize--;
>  	}
>  
>  	rsize = hid_compute_report_size(report);
> @@ -1806,9 +1814,15 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
>  	else if (rsize > max_buffer_size)
>  		rsize = max_buffer_size;
>  
> +	if (bsize < rsize) {
> +		hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n",
> +				     report->id, rsize, bsize);

[Severity: Low]
This isn't a bug, but there's another instance here where bsize is printed
with %ld instead of %zu.

> +		return -EINVAL;
> +	}
> +
>  	if (csize < rsize) {
>  		dbg_hid("report %d is too short, (%d < %d)\n", report->id,
> -				csize, rsize);
> +			csize, rsize);
>  		memset(cdata + csize, 0, rsize - csize);
>  	}

[ ... ]

> @@ -1851,6 +1865,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, u32 size, int i
>  	struct hid_report_enum *report_enum;
>  	struct hid_driver *hdrv;
>  	struct hid_report *report;
> +	size_t bufsize = size;
>  	int ret = 0;
>  
>  	if (!hid)
> @@ -1889,7 +1904,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, u32 size, int i
>  			goto unlock;
>  	}
>  
> -	ret = hid_report_raw_event(hid, type, data, size, interrupt);
> +	ret = hid_report_raw_event(hid, type, data, bufsize, size, interrupt);

[Severity: High]
Does this defeat the short report mitigation for most devices?

Since hid_input_report() is the primary entry point for HID transport
drivers and lacks a bufsize argument, it sets bufsize equal to size (the
payload length). When a short report is received, this guarantees that
bsize < rsize evaluates to true in hid_report_raw_event().

Will this incorrectly drop valid short reports for most HID devices by
triggering the -EINVAL rejection path?

>  
>  unlock:
>  	up(&hid->driver_input_lock);

[ ... ]

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index c8a8daedff3d0..1b130ac45acad 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -79,7 +79,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 the bufsize argument here be sizeof(buf) instead of size?

Because bufsize is being passed as the short data length rather than the
true allocated buffer size, hid_report_raw_event() will evaluate
bsize < rsize as true and incorrectly reject valid short wacom reports
with -EINVAL.

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