[PATCH] HID: uhid: convert to hid_safe_input_report()

[PATCH] HID: uhid: convert to hid_safe_input_report()

[Carlos Llamas]

Commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing
bogus memset()"), added a check in hid_report_raw_event() to reject
reports if the received data size is smaller than expected. This was
intended to prevent OOB errors by no longer allowing zeroing-out of
shorter reports due to the lack of buffer size information.

However, this leads to regressions in hid_report_raw_event(), where
shorter than expected reports are rejected, even though their buffers
are sufficiently large to be zero-padded.

To solve this issue, Benjamin introduced a safer alternative in commit
206342541fc8 ("HID: core: introduce hid_safe_input_report()"), which
forwards the buffer size and allows hid_report_raw_event() to safely
zero-pad the data.

Convert uhid to use hid_safe_input_report() and pass UHID_DATA_MAX as
the buffer size. This prevents the reported regressions [1], allowing
hid core to zero-pad the shorter reports safely as expected.

Cc: stable@vger.kernel.org
Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()")
Closes: https://lore.kernel.org/all/ahsh0UtTX6e0ZeHa@google.com/ [1]
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/hid/uhid.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 524b53a3c87b..37b60c3aaf66 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -595,8 +595,8 @@ static int uhid_dev_input(struct uhid_device *uhid, struct uhid_event *ev)
 	if (!READ_ONCE(uhid->running))
 		return -EINVAL;
 
-	hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data,
-			 min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);
+	hid_safe_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data, UHID_DATA_MAX,
+			      min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);
 
 	return 0;
 }
@@ -606,8 +606,8 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
 	if (!READ_ONCE(uhid->running))
 		return -EINVAL;
 
-	hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input2.data,
-			 min_t(size_t, ev->u.input2.size, UHID_DATA_MAX), 0);
+	hid_safe_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input2.data, UHID_DATA_MAX,
+			      min_t(size_t, ev->u.input2.size, UHID_DATA_MAX), 0);
 
 	return 0;
 }
-- 
2.54.0.1032.g2f8565e1d1-goog

Re: [PATCH] HID: uhid: convert to hid_safe_input_report()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] In `dispatch_hid_bpf_device_event()`, there is no bounds check before copying `*size` bytes into `hdev->bpf.device_data`, leading to an out-of-bounds write if the injected report size exceeds the BPF allocated memory.
--

commit c570c0c551ca1ea46eb86fb2144c1ad379f64069
Author: Carlos Llamas <cmllamas@google.com>

HID: uhid: convert to hid_safe_input_report()

This commit converts uhid to use hid_safe_input_report() and passes
UHID_DATA_MAX as the buffer size. This allows hid core to safely zero-pad
shorter reports instead of rejecting them, preventing regressions caused by
the earlier bogus memset() removal.

Closes: https://lore.kernel.org/all/ahsh0UtTX6e0ZeHa@google.com/

> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 524b53a3c87b3..37b60c3aaf66a 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -595,8 +595,8 @@ static int uhid_dev_input(struct uhid_device *uhid, struct uhid_event *ev)
>  	if (!READ_ONCE(uhid->running))
>  		return -EINVAL;
>  
> -	hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data,
> -			 min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);
> +	hid_safe_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data, UHID_DATA_MAX,
> +			      min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);

[Severity: Critical]
This is a pre-existing issue, but does forwarding this report size without
constraining it to the device's max_report_len cause a buffer overflow in HID
BPF?

In uhid_dev_input(), we allow userspace to inject input reports up to
UHID_DATA_MAX (4096 bytes) regardless of the device's parsed max_report_len.

When a HID-BPF program is attached to the device, hdev->bpf.device_data is
allocated based on the device's max_report_len, which could be much smaller.

Later, in drivers/hid/bpf/hid_bpf_dispatch.c:dispatch_hid_bpf_device_event(),
it executes:

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

Since there is no bounds check on *size before the memcpy(), could a malicious
uhid client injecting a large report size cause a heap buffer overflow when
copying into the smaller BPF allocated memory?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606181552.3095967-1-cmllamas@google.com?part=1