HID: bpf: reject oversized device events before copying

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

Posted by Mark Amirkan via B4 Relay 1 week, 4 days ago

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.

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)
+		return ERR_PTR(-EINVAL);
+
 	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>

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

Posted by Benjamin Tissoires 1 week, 2 days ago

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>
> 
> 
>

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

Posted by bot+bpf-ci@kernel.org 1 week, 3 days ago

> 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.

This isn't a bug, but could the last paragraph be trimmed to one
sentence naming the reproducer, since the KUnit test it refers to is not
part of the patch? The rest of the message clearly states the problem
and the fix, and this final paragraph reads more like a test report than
a changelog - it counts test runs and caveats its own coverage.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34914058900