* Re: [PATCH] HID: bpf: reject oversized device events before copying
2026-09-14 0:15 [PATCH] HID: bpf: reject oversized device events before copying Mark Amirkan via B4 Relay
@ 2026-09-15 1:03 ` bot+bpf-ci
2026-09-15 14:07 ` Benjamin Tissoires
1 sibling, 0 replies; 3+ messages in thread
From: bot+bpf-ci @ 2026-09-15 1:03 UTC (permalink / raw)
To: markdamirkan, jikos, bentiss
Cc: linux-input, linux-kernel, bpf, stable, markdamirkan, ast,
andrii, daniel, martin.lau, eddyz87, yonghong.song, mason,
ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: bpf: reject oversized device events before copying
2026-09-14 0:15 [PATCH] HID: bpf: reject oversized device events before copying Mark Amirkan via B4 Relay
2026-09-15 1:03 ` bot+bpf-ci
@ 2026-09-15 14:07 ` Benjamin Tissoires
1 sibling, 0 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2026-09-15 14:07 UTC (permalink / raw)
To: markdamirkan; +Cc: Jiri Kosina, linux-input, linux-kernel, bpf, stable
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>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread