Re: [linux-6.12.y 2/4] 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 for standard transport drivers because APIs do not propagate the real buffer size, bypassing the newly introduced padding logic.
- [Low] Format string mismatch for `size_t` variable.

Pre-existing issues:
- [Critical] Heap buffer overflow in `dispatch_hid_bpf_device_event` when processing incoming events.
- [High] A BPF program calling the `bpf_hid_input_report` helper causes unhandled re-entrancy that silently corrupts the shared per-device BPF data buffer.
--

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

HID: pass the buffer size to hid_report_raw_event

This commit attempts to fix an OOB mitigation by passing both the buffer
capacity and the actual data size to hid_report_raw_event(). The goal is
to allow safe zero-padding of short reports if the backing buffer is large
enough, rather than unconditionally rejecting them.

[ Upstream commit 2c85c61d1332e1e16f020d76951baf167dcb6f7a ]
[Lee: Backported to linux-6.12.y and beyond]

> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 284861c166d9c..b711d83dfde1d 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -24,7 +24,8 @@ EXPORT_SYMBOL(hid_ops);
>  
>  u8 *
>  dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data,
> -			      u32 *size, int interrupt, u64 source, bool from_bpf)
> +			      size_t *buf_size, u32 *size, int interrupt, u64 source,
> +			      bool from_bpf)
>  {

[Severity: Critical]
This is a pre-existing issue, but does dispatch_hid_bpf_device_event() lack
bounds checking on the event size?

When a HID event arrives, the function unconditionally copies the data into the
BPF context buffer:

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

If a malicious or compromised HID device (or userspace via uhid) sends an
event where *size is larger than hdev->bpf.allocated_data, could this cause a
heap buffer overflow?

[Severity: High]
This is also a pre-existing issue, but does this code suffer from unhandled
re-entrancy corrupting shared state?

If a BPF program injects an input report by calling the bpf_hid_input_report()
helper, execution re-enters dispatch_hid_bpf_device_event(). The inner call
will unconditionally execute the memset() and memcpy() block shown above on
the exact same shared hdev->bpf.device_data buffer.

If the injected report uses ctx->data as its buffer, the memset wipes the data
before it can be copied. Will this silently corrupt the execution state of the
outer BPF program upon return?

[ ... ]
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 294a25330ed03..ceff91722c3c8 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
[ ... ]
> @@ -2028,9 +2036,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);

[Severity: Low]
Can we use the %zu format specifier instead of %ld for bsize?

bsize is a size_t, which might trigger a compiler warning on 32-bit platforms
where size_t is unsigned int.

> +		return -EINVAL;
> +	}
> +
[ ... ]
> @@ -2135,7 +2150,7 @@ static int __hid_input_report(struct hid_device *hid, enum hid_report_type type,
>  int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size,
>  		     int interrupt)
>  {
> -	return __hid_input_report(hid, type, data, size, interrupt, 0,
> +	return __hid_input_report(hid, type, data, size, size, interrupt, 0,

[Severity: High]
Does this leave the short-report regression unfixed for standard transport
drivers?

By blindly passing the received data size as both the size and bufsize capacity
arguments to __hid_input_report(), bsize will always equal size.

If the report is short (size < rsize), the new bsize < rsize condition in
hid_report_raw_event() evaluates to true and the event is dropped, bypassing
the padding logic completely.

>  				  false, /* from_bpf */
>  				  false /* lock_already_taken */);
>  }
[ ... ]
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 1b1112772777c..ffcf65dcf7134 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]
Does passing size instead of sizeof(buf) for the buffer capacity prevent safe
padding for wacom devices?

Similar to the issue above, because bsize becomes equal to size, any short
report will fail the bsize < rsize check in hid_report_raw_event() and be
rejected instead of zeroes being safely padded to the end of the buffer.

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