Re: [PATCH] HID: bpf: reject oversized device events before copying

[Benjamin Tissoires <bentiss@kernel.org>]

Hi Mark,

Couple of comments:

On Sep 13 2026, Mark Amirkan via B4 Relay wrote:
> From: Mark Amirkan <markdamirkan@gmail.com>
> 
> HID-BPF allocates its device-event buffer from the largest report in the
> device's parsed report descriptor.  dispatch_hid_bpf_device_event() then
> copies the transport-provided report into that buffer before checking its
> size.
> 
> An input report larger than the allocated event buffer therefore causes
> a heap out-of-bounds write when a device-event program is attached.  The
> later check of the BPF program's return value cannot prevent the initial
> copy.
> 
> Reject reports that exceed either the transport buffer or the persistent
> HID-BPF event buffer before clearing or copying the data.
> 
> With a 64-byte event allocation, a same-file KUnit test produced a
> one-byte KASAN out-of-bounds write for a 65-byte report in three runs.
> The 64-byte boundary remained clean.  After this change, both cases were
> clean in three runs and the oversized report returned -EINVAL.  The test
> exercised the production dispatch function but not the complete UHID and
> BPF attachment path.

As mentioned by the bpf bot: if you already have tests, why not upstream
them? And even better: there are already selftests at
tools/testing/selftests/hid/hid_bpf.c -> why not add some more here as
well?

> 
> Fixes: 658ee5a64fcf ("HID: bpf: allocate data memory for device_event BPF programs")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM Symbolic
> Signed-off-by: Mark Amirkan <markdamirkan@gmail.com>
> ---
>  drivers/hid/bpf/hid_bpf_dispatch.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 536f6d01fd..06ba833eb9 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -50,6 +50,9 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
>  	if (!hdev->bpf.device_data)
>  		return data;
>  
> +	if (*size > *buf_size || *size > ctx_kern.ctx.allocated_size)

I had a hard time figuring out why `*size > *buf_size` wasn't there in
the first place. You can call it overlooked, but:
- when the call chain of dispatch_hid_bpf_device_event() contains
	hid_input_report() -> *size == *buf_size
- when the call chain contains hid_safe_input_report(), the bufsize and
	size are forwarded from the lower transport drivers.

We only have 3 callers of hid_safe_input_report():
- uhid.c -> min_t(size_t, ev->u.input.size, UHID_DATA_MAX) is present
- i2c-hid-core.c -> size can'be greater than bufsize because it either
	truncate the input report when I2C_HID_QUIRK_BAD_INPUT_SIZE is set or
	plainly reject too long size
- usbhid/hid-core.c -> we rely on usb to set the size and the transfer
	buffer, so it should be OK

Regarding `*size > ctx_kern.ctx.allocated_size`:
- usbhid/i2c-hid: both are allocating their transfert buffer through the
	exact same call to hid_report_len() and finding the biggest of all reports
	-> so they are not affected
- uhid: here we don't control really what's going on, because size is a
	user provided parameter.

All in all: the first test is IMO probably redundant. Maybe we can just
add a comment in hid_safe_input_report() that the caller must ensure
`size <= bufsize`.

The second one matters only for the uhid case, so technically yes, we
can demonstrate a malicious OOB with uhid (but you should already be
root to create/controll a uhid device).

> +		return ERR_PTR(-EINVAL);

The whole point of hid-bpf is to fix bogus devices. So plainly returning
-EINVAL means we don't even let the user the chance to fix the device.

Later, hid_report_raw_event() has a similar check and when size is too
big, it would also break the processing but at least give a warning in
the dmesg.

So, how about we conditionally memcpy the size to min(*size,
allocated_size), and let the bpf change the size later on by itself ?
This should allow a HID-BPF program to amend the provided size on a
given device. This works because hid_bpf_get_data() doesn't know about
.size, but only .allocated_size, so the kernel will prevent a BPF to
access beyond the allocated_size.


TL;DR: please,
- make a decision regarding "*size > *buf_size"
- change memcpy(size) into memcpy(min(size, allocated_size))
- add tests in selftests

Cheers,
Benjamin

> +
>  	memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
>  	memcpy(ctx_kern.data, data, *size);
>  
> 
> ---
> base-commit: 9cdc7e6dc7a99ad7311ad5e7c145f2b9ce4e24b0
> change-id: 20260913-b4-send-hid-bpf-event-bounds-4d0e8170ceb4
> 
> Best regards,
> --  
> Mark Amirkan <markdamirkan@gmail.com>
> 
> 
>