Re: [linux-5.10.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 2 potential issue(s) to consider:
- [High] The patch introduces `bufsize` to `hid_report_raw_event` to allow safe zero-padding of short reports. However, it passes `size` (the received data length) instead of the actual buffer capacity in key call sites. Most notably, in `wacom_wac_queue_flush()`, the buffer has a fixed capacity (`u8 buf[WACOM_PKGLEN_MAX]`), but `size` is incorrectly passed as `bufsize`. In `hid_input_report()`, `bufsize` is hardcoded to `size`. This guarantees that `bsize == csize`, meaning any short report (`csize < rsize`) will unconditionally trigger the `if (bsize < rsize)` error check and be dropped with `-EINVAL`. The intended zero-padding mitigation (`memset`) is therefore completely unreachable for these paths.
- [Low] Format specifier mismatch in error logs: `bsize` is of type `size_t` but is printed using `%ld` instead of `%zu`. Additionally, unsigned 32-bit variables (`report->id`, `csize`, `rsize`) are printed using the signed `%d` specifier instead of `%u`.
--

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

HID: pass the buffer size to hid_report_raw_event

This commit updates hid_report_raw_event to accept both the buffer size and
the data size. This allows the function to make a better decision about
whether to reject a buffer to prevent an overflow, or to safely pad it with
zeroes.

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index aa9ae6ccb28a8..c73f4ac16fdf2 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -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]
This isn't a bug, but could the format specifiers cause compiler warnings on
32-bit architectures? bsize is a size_t and csize and report->id are unsigned,
so should they use %zu and %u instead of %ld and %d?

> +		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]
Similarly here, should this use %u for report->id and rsize, and %zu for
bsize instead of %d and %ld?

> +		return -EINVAL;
> +	}

[Severity: High]
If callers pass the incoming data length as both the size and bufsize
arguments, does this mean bsize will equal csize?

If bsize == csize, and we receive a short report where csize < rsize,
wouldn't the bsize < rsize check just above this evaluate to true and return
-EINVAL? Does this make the memset padding logic unreachable for those paths?

> +
>  	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 setting bufsize directly to size here defeat the padding mitigation?

This hardcodes bufsize to the received payload length, which propagates the
drop behavior to all transport drivers relying on hid_input_report().

[ ... ]

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index abbfb53bb7dc9..09b513812fff1 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]
Could we pass sizeof(buf) here instead of size?

Since buf has a fixed capacity of WACOM_PKGLEN_MAX, passing size for
bufsize prevents the core from safely padding short reports and will
instead cause them to be dropped.

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