Re: [linux-6.1.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 1 potential issue(s) to consider:
- [High] The logic to safely pad short HID reports with zeros is entirely unreachable for standard input reports, causing valid short reports to be incorrectly rejected.
--

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

HID: pass the buffer size to hid_report_raw_event

This commit introduces a bufsize parameter to hid_report_raw_event() to help
distinguish between the actual payload size and the buffer capacity. This allows
the HID core to safely memset zero-pad short reports without causing
out-of-bounds writes if the buffer is too small.

> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
[ ... ]
> @@ -2019,9 +2027,15 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
>  	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);
> +		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);
>  	}

[Severity: High]
Does this new check unconditionally reject short reports for callers that pass
the payload size as the buffer size?

Since bsize is checked against rsize and returns -EINVAL, if bsize
happens to equal csize, the subsequent csize < rsize check and its memset
zero-padding logic become unreachable dead code.

[ ... ]
> @@ -2058,12 +2072,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;
>  	int ret = 0;
>  
>  	if (!hid)

[Severity: High]
Does initializing bufsize to size here lose the actual allocation capacity
of the buffer?

By constraining bufsize to the payload size, this ensures that bsize == csize
in hid_report_raw_event(), which means valid short reports missing trailing
zero bytes will be rejected rather than padded.

[ ... ]
> --- 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?

Since size is the number of bytes read from the kfifo (the payload size),
passing size as both the buffer size and data size prevents the short-report
zero-padding logic from working correctly.

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