[PATCH 0/3] HID: bpf: fix __hid_bpf_hw_check_params report length computation

[PATCH 0/3] HID: bpf: fix __hid_bpf_hw_check_params report length computation

[Benjamin Tissoires]

While playing with hid_bpf_hw_output_report() on a device without report
IDs, I realized I wasn't able to send proper reports to the USB
endpoint.

This is because the __hid_bpf_hw_check_params() check wasn't taking into
account the report ID reserved space, and it was trimming down one
element in the report.

Of course, things are not simple, because sending an input report is not
taking this extra space into account, so we need to account for the type
of request as well.

Cheers,
Benjamin

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (3):
      selftests/hid: add define for commonly used buf size
      HID: bpf: fix __hid_bpf_hw_check_params report length
      selftests/hid: add unnumbered variant to the hid_bpf tests

 drivers/hid/bpf/hid_bpf_dispatch.c       | 12 +++--
 tools/testing/selftests/hid/hid_bpf.c    | 91 ++++++++++++++++++++++----------
 tools/testing/selftests/hid/hid_common.h | 25 ++++++++-
 tools/testing/selftests/hid/progs/hid.c  |  2 +-
 4 files changed, 96 insertions(+), 34 deletions(-)
---
base-commit: a93f3bf4e1d60777b1659b812c9e818cfc53b449
change-id: 20260904-wip-bpf-check_report-f490de16b864

Best regards,
--  
Benjamin Tissoires <bentiss@kernel.org>

[PATCH 1/3] selftests/hid: add define for commonly used buf size

[Benjamin Tissoires]

If we want to add another report descriptor without report IDs with a
report size bigger than 10, we have multiple magic values to replace.

Put a #define once and for all, so we don't have dangling ones.

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 tools/testing/selftests/hid/hid_bpf.c    | 38 ++++++++++++++++----------------
 tools/testing/selftests/hid/hid_common.h |  3 ++-
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index b851339308c2..e865ee55b4f1 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -5,7 +5,7 @@
 #include <bpf/bpf.h>
 
 struct hid_hw_request_syscall_args {
-	__u8 data[10];
+	__u8 data[MAX_BUF_SIZE];
 	unsigned int hid;
 	int retval;
 	size_t size;
@@ -168,7 +168,7 @@ TEST_F(hid_bpf, raw_event)
 	const struct test_program progs[] = {
 		{ .name = "hid_first_event" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -219,7 +219,7 @@ TEST_F(hid_bpf, subprog_raw_event)
 	const struct test_program progs[] = {
 		{ .name = "hid_subprog_first_event" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -277,7 +277,7 @@ TEST_F(hid_bpf, test_attach_detach)
 		{ .name = "hid_second_event" },
 	};
 	struct bpf_link *link;
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err, link_fd;
 
 	LOAD_PROGRAMS(progs);
@@ -362,7 +362,7 @@ TEST_F(hid_bpf, test_hid_change_report)
 	const struct test_program progs[] = {
 		{ .name = "hid_change_report_id" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -389,13 +389,13 @@ TEST_F(hid_bpf, test_hid_user_input_report_call)
 {
 	struct hid_hw_request_syscall_args args = {
 		.retval = -1,
-		.size = 10,
+		.size = MAX_BUF_SIZE,
 	};
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
 			    .ctx_in = &args,
 			    .ctx_size_in = sizeof(args),
 	);
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err, prog_fd;
 
 	LOAD_BPF;
@@ -435,7 +435,7 @@ TEST_F(hid_bpf, test_hid_user_output_report_call)
 {
 	struct hid_hw_request_syscall_args args = {
 		.retval = -1,
-		.size = 10,
+		.size = MAX_BUF_SIZE,
 	};
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
 			    .ctx_in = &args,
@@ -484,7 +484,7 @@ TEST_F(hid_bpf, test_hid_user_raw_request_call)
 		.retval = -1,
 		.type = HID_FEATURE_REPORT,
 		.request_type = HID_REQ_GET_REPORT,
-		.size = 10,
+		.size = MAX_BUF_SIZE,
 	};
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
 			    .ctx_in = &args,
@@ -517,7 +517,7 @@ TEST_F(hid_bpf, test_hid_filter_raw_request_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_filter_raw_request" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -570,7 +570,7 @@ TEST_F(hid_bpf, test_hid_change_raw_request_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_hidraw_raw_request" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -596,7 +596,7 @@ TEST_F(hid_bpf, test_hid_infinite_loop_raw_request_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_infinite_loop_raw_request" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -619,7 +619,7 @@ TEST_F(hid_bpf, test_hid_filter_output_report_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_filter_output_report" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -672,7 +672,7 @@ TEST_F(hid_bpf, test_hid_change_output_report_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_hidraw_output_report" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -696,7 +696,7 @@ TEST_F(hid_bpf, test_hid_infinite_loop_output_report_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_infinite_loop_output_report" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -722,7 +722,7 @@ TEST_F(hid_bpf, test_multiply_events_wq)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_multiply_events_wq" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -760,7 +760,7 @@ TEST_F(hid_bpf, test_multiply_events)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_multiply_events" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -794,7 +794,7 @@ TEST_F(hid_bpf, test_hid_infinite_loop_input_report_call)
 	const struct test_program progs[] = {
 		{ .name = "hid_test_infinite_loop_input_report" },
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
@@ -848,7 +848,7 @@ TEST_F(hid_bpf, test_hid_attach_flags)
 			.insert_head = 0,
 		},
 	};
-	__u8 buf[10] = {0};
+	__u8 buf[MAX_BUF_SIZE] = {0};
 	int err;
 
 	LOAD_PROGRAMS(progs);
diff --git a/tools/testing/selftests/hid/hid_common.h b/tools/testing/selftests/hid/hid_common.h
index e3b267446fa0..4567336f131d 100644
--- a/tools/testing/selftests/hid/hid_common.h
+++ b/tools/testing/selftests/hid/hid_common.h
@@ -13,6 +13,7 @@
 #include <linux/uhid.h>
 
 #define SHOW_UHID_DEBUG 0
+#define MAX_BUF_SIZE 10
 
 #define min(a, b) \
 	({ __typeof__(a) _a = (a); \
@@ -110,7 +111,7 @@ static pthread_cond_t uhid_started = PTHREAD_COND_INITIALIZER;
 
 static pthread_mutex_t uhid_output_mtx = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t uhid_output_cond = PTHREAD_COND_INITIALIZER;
-static unsigned char output_report[10];
+static unsigned char output_report[MAX_BUF_SIZE];
 
 /* no need to protect uhid_stopped, only one thread accesses it */
 static bool uhid_stopped;

-- 
2.55.0

[PATCH 2/3] HID: bpf: fix __hid_bpf_hw_check_params report length

[Benjamin Tissoires]

Turns out that USB, I2C and other transport drivers (except uhid which
just passes the data) still need to have the report ID in the first
byte.

Because they expect the first byte to be the report ID or 0, when the
report ID is 0, they strip that first byte before forwarding to the
device. This means that the transport layer forwards a buffer of size
N-1 to the device, which gets rejected.

Fixes: 5599f8019661 ("HID: bpf: export hid_hw_output_report as a BPF kfunc")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 drivers/hid/bpf/hid_bpf_dispatch.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index 536f6d01fd14..b1de1dd0f21d 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -359,7 +359,7 @@ hid_bpf_release_context(struct hid_bpf_ctx *ctx)
 
 static int
 __hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
-			  enum hid_report_type rtype)
+			  enum hid_report_type rtype, bool hw_request)
 {
 	struct hid_report_enum *report_enum;
 	struct hid_report *report;
@@ -388,6 +388,10 @@ __hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
 
 	report_len = hid_report_len(report);
 
+	/* unnumbered reports need to have a report ID reserved in the first byte */
+	if (hw_request && report_enum->numbered == 0)
+		report_len += 1;
+
 	if (*buf__sz > report_len)
 		*buf__sz = report_len;
 
@@ -420,7 +424,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
 		return -EDEADLOCK;
 
 	/* check arguments */
-	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
+	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
 	if (ret)
 		return ret;
 
@@ -480,7 +484,7 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
 		return -EDEADLOCK;
 
 	/* check arguments */
-	ret = __hid_bpf_hw_check_params(ctx, buf, &size, HID_OUTPUT_REPORT);
+	ret = __hid_bpf_hw_check_params(ctx, buf, &size, HID_OUTPUT_REPORT, true);
 	if (ret)
 		return ret;
 
@@ -506,7 +510,7 @@ __hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *b
 		return -EDEADLOCK;
 
 	/* check arguments */
-	ret = __hid_bpf_hw_check_params(ctx, buf, &size, type);
+	ret = __hid_bpf_hw_check_params(ctx, buf, &size, type, false);
 	if (ret)
 		return ret;
 

-- 
2.55.0

[PATCH 3/3] selftests/hid: add unnumbered variant to the hid_bpf tests

[Benjamin Tissoires]

A bug appeared in hid_bpf_dispatch.c where it wasn't properly handling
unnumbered reports. Add a device variant without report IDs so we can
also test them.

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 tools/testing/selftests/hid/hid_bpf.c    | 53 ++++++++++++++++++++++++++------
 tools/testing/selftests/hid/hid_common.h | 24 ++++++++++++++-
 tools/testing/selftests/hid/progs/hid.c  |  2 +-
 3 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index e865ee55b4f1..d1bd07caf991 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -54,11 +54,27 @@ FIXTURE_TEARDOWN(hid_bpf) {
 	hid_bpf_teardown(_metadata, self, variant); \
 } while (0)
 
+FIXTURE_VARIANT(hid_bpf) {
+	__u8 *rdesc;
+	size_t rdesc_size;
+};
+
+FIXTURE_VARIANT_ADD(hid_bpf, numbered) {
+	.rdesc = rdesc,
+	.rdesc_size = sizeof(rdesc),
+};
+
+FIXTURE_VARIANT_ADD(hid_bpf, unnumbered) {
+	.rdesc = fido2_rdesc,
+	.rdesc_size = sizeof(fido2_rdesc),
+};
+
 FIXTURE_SETUP(hid_bpf)
 {
 	int err;
 
-	err = setup_uhid(_metadata, &self->hid, BUS_USB, 0x0001, 0x0a36, rdesc, sizeof(rdesc));
+	err = setup_uhid(_metadata, &self->hid, BUS_USB, 0x0001, 0x0a36,
+			 variant->rdesc, variant->rdesc_size);
 	ASSERT_OK(err);
 }
 
@@ -402,8 +418,11 @@ TEST_F(hid_bpf, test_hid_user_input_report_call)
 
 	args.hid = self->hid.hid_id;
 	args.data[0] = 1; /* report ID */
-	args.data[1] = 2; /* report ID */
-	args.data[2] = 42; /* report ID */
+	args.data[1] = 2;
+	args.data[2] = 42;
+
+	if (variant->rdesc == fido2_rdesc)
+		args.data[0] = 0;
 
 	prog_fd = bpf_program__fd(self->skel->progs.hid_user_input_report);
 
@@ -421,8 +440,13 @@ TEST_F(hid_bpf, test_hid_user_input_report_call)
 	/* read the data from hidraw */
 	memset(buf, 0, sizeof(buf));
 	err = read(self->hidraw_fd, buf, sizeof(buf));
-	ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
-	ASSERT_EQ(buf[0], 1);
+	if (variant->rdesc == rdesc) {
+		ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+	} else {
+		ASSERT_EQ(err, 64)
+		TH_LOG("read_hidraw");
+	}
+	ASSERT_EQ(buf[0], args.data[0]);
 	ASSERT_EQ(buf[1], 2);
 	ASSERT_EQ(buf[2], 42);
 }
@@ -448,8 +472,11 @@ TEST_F(hid_bpf, test_hid_user_output_report_call)
 
 	args.hid = self->hid.hid_id;
 	args.data[0] = 1; /* report ID */
-	args.data[1] = 2; /* report ID */
-	args.data[2] = 42; /* report ID */
+	args.data[1] = 2;
+	args.data[2] = 42;
+
+	if (variant->rdesc == fido2_rdesc)
+		args.data[0] = 0;
 
 	prog_fd = bpf_program__fd(self->skel->progs.hid_user_output_report);
 
@@ -465,9 +492,14 @@ TEST_F(hid_bpf, test_hid_user_output_report_call)
 	ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
 	ASSERT_OK(cond_err) TH_LOG("error while calling waiting for the condition");
 
-	ASSERT_EQ(args.retval, 3);
+	if (variant->rdesc == rdesc) {
+		ASSERT_EQ(args.retval, 3);
+	} else if (variant->rdesc == fido2_rdesc) {
+		ASSERT_EQ(args.retval, 65)
+		TH_LOG("report size error, should have 64 + 1 extra byte for the report ID 0");
+	}
 
-	ASSERT_EQ(output_report[0], 1);
+	ASSERT_EQ(output_report[0], args.data[0]);
 	ASSERT_EQ(output_report[1], 2);
 	ASSERT_EQ(output_report[2], 42);
 
@@ -879,6 +911,9 @@ TEST_F(hid_bpf, test_rdesc_fixup)
 	};
 	int err, desc_size;
 
+	if (variant->rdesc != rdesc)
+		SKIP(return, "not compatible report descriptor");
+
 	LOAD_PROGRAMS(progs);
 
 	/* check that hid_rdesc_fixup() was executed */
diff --git a/tools/testing/selftests/hid/hid_common.h b/tools/testing/selftests/hid/hid_common.h
index 4567336f131d..b7890ba2878f 100644
--- a/tools/testing/selftests/hid/hid_common.h
+++ b/tools/testing/selftests/hid/hid_common.h
@@ -13,7 +13,7 @@
 #include <linux/uhid.h>
 
 #define SHOW_UHID_DEBUG 0
-#define MAX_BUF_SIZE 10
+#define MAX_BUF_SIZE 128
 
 #define min(a, b) \
 	({ __typeof__(a) _a = (a); \
@@ -98,6 +98,28 @@ static unsigned char rdesc[] = {
 
 static __u8 feature_data[] = { 1, 2 };
 
+static __maybe_unused unsigned char fido2_rdesc[] = {
+	0x06, 0xd0, 0xf1,	/* Usage Page (FIDO Alliance) */
+	0x09, 0x01,		/* Usage (U2F Authenticator Device) */
+	0xa1, 0x01,		/* Collection (Application) */
+	0x09, 0x20,		/*  Usage (Input Report Data) */
+	0x15, 0x00,		/*  Logical Minimum (0) */
+	0x26, 0xff, 0x00,	/*  Logical Maximum (255) */
+	0x75, 0x08,		/*  Report Size (8) */
+	0x95, 0x40,		/*  Report Count (64) */
+	0x81, 0x02,		/*  Input (Data,Var,Abs) */
+	0x09, 0x21,		/*  Usage (Output Report Data) */
+	0x15, 0x00,		/*  Logical Minimum (0) */
+	0x26, 0xff, 0x00,	/*  Logical Maximum (255) */
+	0x75, 0x08,		/*  Report Size (8) */
+	0x95, 0x40,		/*  Report Count (64) */
+	0x91, 0x02,		/*  Output (Data,Var,Abs) */
+	0x06, 0x00, 0xff,	/*  Usage Page (Vendor Defined Page 1) */
+	0x09, 0x22,		/*  Usage (Vendor Usage 0x22) */
+	0xb1, 0x02,		/*  Feature (Data,Var,Abs) */
+	0xc0,			/* End Collection */
+};
+
 #define ASSERT_OK(data) ASSERT_FALSE(data)
 #define ASSERT_OK_PTR(ptr) ASSERT_NE(NULL, ptr)
 
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index b21fbb13c926..605d8a5f9cb2 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -98,7 +98,7 @@ struct hid_bpf_ops change_report_id = {
 
 struct hid_hw_request_syscall_args {
 	/* data needs to come at offset 0 so we can use it in calls */
-	__u8 data[10];
+	__u8 data[128];
 	unsigned int hid;
 	int retval;
 	size_t size;

-- 
2.55.0

Re: [PATCH 2/3] HID: bpf: fix __hid_bpf_hw_check_params report length

[sashiko-bot]

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

New issues:
- [Medium] The logic to reserve an extra byte for the report ID incorrectly checks `report_enum->numbered == 0` instead of `report->id == 0`, leading to buffer truncation for unnumbered reports on devices that also declare numbered reports.

Pre-existing issues:
- [High] An integer sign mismatch in `hid_bpf_hw_request` causes negative error codes returned by the transport driver to be cast to `size_t` and evaluated as large positive values, incorrectly converting hardware errors into a 'success' return value.
--

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

HID: bpf: fix __hid_bpf_hw_check_params report length
    
This patch modifies __hid_bpf_hw_check_params to reserve an extra byte for
the report ID during hardware requests on unnumbered reports. This ensures
transport drivers do not strip the first byte of the payload and reject the
request due to insufficient buffer size.

> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 536f6d01fd14c..b1de1dd0f21d0 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
[ ... ]
> @@ -388,6 +388,10 @@ __hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
>  
>  	report_len = hid_report_len(report);
>  
> +	/* unnumbered reports need to have a report ID reserved in the first byte */
> +	if (hw_request && report_enum->numbered == 0)
> +		report_len += 1;
> +

[Severity: Medium]
Does this correctly handle devices that declare both unnumbered and numbered
reports?

If a non-compliant HID device declares an unnumbered report where the report
ID is 0, but also declares other numbered reports, report_enum->numbered
will be set to 1.

When a BPF program attempts a hardware request for the unnumbered report on
such a device, the report_enum->numbered == 0 check will evaluate to false
and skip the +1 padding. The BPF-provided buffer size is then clamped, but
the transport driver will still strip the first byte before transmission,
leading to buffer truncation.

Would it be safer to check if report->id == 0 instead?

>  	if (*buf__sz > report_len)
>  		*buf__sz = report_len;
>  
[ ... ]
> @@ -420,7 +424,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
>  		return -EDEADLOCK;
>  
>  	/* check arguments */
> -	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
> +	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
>  	if (ret)
>  		return ret;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential integer sign mismatch
in hid_bpf_hw_request involving the size variable passed here.

In hid_bpf_hw_request, size is defined as a size_t, while ret is an int.
Later in the function, after hid_hw_raw_request returns its status into ret,
the code checks:

    if (ret > size)
        ret = size;
    if (ret > 0)
        memcpy(buf, dma_data, ret);

If a transport driver encounters a hardware error and returns a negative
error code such as -EPIPE, ret is implicitly promoted to size_t when compared
against size. This causes the negative error code to evaluate as a large
positive number, making the (ret > size) condition true.

This silently converts the hardware error into a success return value and
copies the unmodified buffer back, which could leave BPF programs operating
on uninitialized or stale data.

Can this be addressed by casting size to an ssize_t during the comparison, or
by handling negative values in ret before the comparison?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wip-bpf-check_report-v1-0-efe9a2a1ed28@kernel.org?part=2

Re: [PATCH 2/3] HID: bpf: fix __hid_bpf_hw_check_params report length

[Benjamin Tissoires]

On Sep 04 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The logic to reserve an extra byte for the report ID incorrectly checks `report_enum->numbered == 0` instead of `report->id == 0`, leading to buffer truncation for unnumbered reports on devices that also declare numbered reports.
> 
> Pre-existing issues:
> - [High] An integer sign mismatch in `hid_bpf_hw_request` causes negative error codes returned by the transport driver to be cast to `size_t` and evaluated as large positive values, incorrectly converting hardware errors into a 'success' return value.
> --
> 
> commit 2abe2f741986f5f3130a3dadda6bf82df9cbea21
> Author: Benjamin Tissoires <bentiss@kernel.org>
> 
> HID: bpf: fix __hid_bpf_hw_check_params report length
>     
> This patch modifies __hid_bpf_hw_check_params to reserve an extra byte for
> the report ID during hardware requests on unnumbered reports. This ensures
> transport drivers do not strip the first byte of the payload and reject the
> request due to insufficient buffer size.
> 
> > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> > index 536f6d01fd14c..b1de1dd0f21d0 100644
> > --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> [ ... ]
> > @@ -388,6 +388,10 @@ __hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
> >  
> >  	report_len = hid_report_len(report);
> >  
> > +	/* unnumbered reports need to have a report ID reserved in the first byte */
> > +	if (hw_request && report_enum->numbered == 0)
> > +		report_len += 1;
> > +
> 
> [Severity: Medium]
> Does this correctly handle devices that declare both unnumbered and numbered
> reports?
> 
> If a non-compliant HID device declares an unnumbered report where the report
> ID is 0, but also declares other numbered reports, report_enum->numbered
> will be set to 1.

Sashiko is wrong here. If a HID device declares a device with a report
ID of 0, the report will be tagged as numbered. The report *will* have
space for the report ID, and so there won't be any issues.

> 
> When a BPF program attempts a hardware request for the unnumbered report on
> such a device, the report_enum->numbered == 0 check will evaluate to false
> and skip the +1 padding. The BPF-provided buffer size is then clamped, but
> the transport driver will still strip the first byte before transmission,
> leading to buffer truncation.
> 
> Would it be safer to check if report->id == 0 instead?
> 
> >  	if (*buf__sz > report_len)
> >  		*buf__sz = report_len;
> >  
> [ ... ]
> > @@ -420,7 +424,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
> >  		return -EDEADLOCK;
> >  
> >  	/* check arguments */
> > -	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
> > +	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
> >  	if (ret)
> >  		return ret;
> >  
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential integer sign mismatch
> in hid_bpf_hw_request involving the size variable passed here.
> 
> In hid_bpf_hw_request, size is defined as a size_t, while ret is an int.
> Later in the function, after hid_hw_raw_request returns its status into ret,
> the code checks:
> 
>     if (ret > size)
>         ret = size;
>     if (ret > 0)
>         memcpy(buf, dma_data, ret);
> 
> If a transport driver encounters a hardware error and returns a negative
> error code such as -EPIPE, ret is implicitly promoted to size_t when compared
> against size. This causes the negative error code to evaluate as a large
> positive number, making the (ret > size) condition true.
> 
> This silently converts the hardware error into a success return value and
> copies the unmodified buffer back, which could leave BPF programs operating
> on uninitialized or stale data.
> 
> Can this be addressed by casting size to an ssize_t during the comparison, or
> by handling negative values in ret before the comparison?

Damn, this one seems correct.

Cheers,
Benjamin

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wip-bpf-check_report-v1-0-efe9a2a1ed28@kernel.org?part=2
> 

Re: [PATCH 3/3] selftests/hid: add unnumbered variant to the hid_bpf tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added unnumbered test variant runs against unadapted tests, leading to test suite regressions and false passes.
--

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

selftests/hid: add unnumbered variant to the hid_bpf tests

A bug appeared in hid_bpf_dispatch.c where it wasn't properly handling
unnumbered reports. Add a device variant without report IDs so we can
also test them.

> diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> index e865ee55b4f1d..d1bd07caf9919 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
> @@ -54,11 +54,27 @@ FIXTURE_TEARDOWN(hid_bpf) {
>  	hid_bpf_teardown(_metadata, self, variant); \
>  } while (0)
>  
> +FIXTURE_VARIANT(hid_bpf) {
> +	__u8 *rdesc;
> +	size_t rdesc_size;
> +};
> +
> +FIXTURE_VARIANT_ADD(hid_bpf, numbered) {
> +	.rdesc = rdesc,
> +	.rdesc_size = sizeof(rdesc),
> +};
> +
> +FIXTURE_VARIANT_ADD(hid_bpf, unnumbered) {
> +	.rdesc = fido2_rdesc,
> +	.rdesc_size = sizeof(fido2_rdesc),
> +};
> +

[Severity: Medium]
Will unadapted tests such as raw_event() and
test_hid_filter_raw_request_call() fail when run against this new unnumbered
variant?

Since FIXTURE_VARIANT_ADD registers this variant for all tests in the hid_bpf
fixture, tests that weren't adapted might now fail because they still hardcode
assumptions about the 6-byte numbered report. For instance, raw_event()
expects exactly 6 bytes:

	err = read(self->hidraw_fd, buf, sizeof(buf));
	ASSERT_EQ(err, 6) TH_LOG("read_hidraw");

test_hid_user_raw_request_call() hardcodes a report ID of 1:

	args.hid = self->hid.hid_id;
	args.data[0] = 1; /* report ID */

And test_hid_filter_raw_request_call() injects a 6-byte event:

	buf[0] = 1;
	buf[1] = 42;
	uhid_send_event(_metadata, &self->hid, buf, 6);

Do these other tests need similar if/else logic for variant->rdesc, or
should they explicitly call SKIP() for the unnumbered variant like
test_rdesc_fixup() does?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wip-bpf-check_report-v1-0-efe9a2a1ed28@kernel.org?part=3

Re: [PATCH 3/3] selftests/hid: add unnumbered variant to the hid_bpf tests

[Benjamin Tissoires]

On Sep 04 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added unnumbered test variant runs against unadapted tests, leading to test suite regressions and false passes.
> --
> 
> commit 25b4cfc0df88875655d4c7a83e9318718c0ae97f
> Author: Benjamin Tissoires <bentiss@kernel.org>
> 
> selftests/hid: add unnumbered variant to the hid_bpf tests
> 
> A bug appeared in hid_bpf_dispatch.c where it wasn't properly handling
> unnumbered reports. Add a device variant without report IDs so we can
> also test them.
> 
> > diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> > index e865ee55b4f1d..d1bd07caf9919 100644
> > --- a/tools/testing/selftests/hid/hid_bpf.c
> > +++ b/tools/testing/selftests/hid/hid_bpf.c
> > @@ -54,11 +54,27 @@ FIXTURE_TEARDOWN(hid_bpf) {
> >  	hid_bpf_teardown(_metadata, self, variant); \
> >  } while (0)
> >  
> > +FIXTURE_VARIANT(hid_bpf) {
> > +	__u8 *rdesc;
> > +	size_t rdesc_size;
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(hid_bpf, numbered) {
> > +	.rdesc = rdesc,
> > +	.rdesc_size = sizeof(rdesc),
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(hid_bpf, unnumbered) {
> > +	.rdesc = fido2_rdesc,
> > +	.rdesc_size = sizeof(fido2_rdesc),
> > +};
> > +
> 
> [Severity: Medium]
> Will unadapted tests such as raw_event() and
> test_hid_filter_raw_request_call() fail when run against this new unnumbered
> variant?

If they would have failed, I wouldn't have sent the series :)

> 
> Since FIXTURE_VARIANT_ADD registers this variant for all tests in the hid_bpf
> fixture, tests that weren't adapted might now fail because they still hardcode
> assumptions about the 6-byte numbered report. For instance, raw_event()
> expects exactly 6 bytes:
> 
> 	err = read(self->hidraw_fd, buf, sizeof(buf));
> 	ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
> 
> test_hid_user_raw_request_call() hardcodes a report ID of 1:
> 
> 	args.hid = self->hid.hid_id;
> 	args.data[0] = 1; /* report ID */

report ID is a convention, but if there is no report ID, then it's just
the first byte.

> 
> And test_hid_filter_raw_request_call() injects a 6-byte event:
> 
> 	buf[0] = 1;
> 	buf[1] = 42;
> 	uhid_send_event(_metadata, &self->hid, buf, 6);
> 
> Do these other tests need similar if/else logic for variant->rdesc, or
> should they explicitly call SKIP() for the unnumbered variant like
> test_rdesc_fixup() does?

It works, because uhid is lazy enough to not check against the report
descriptor and just forwards what goes in. So we are just checking the
kernel is not tampering with the data.

Cheers,
Benjamin

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wip-bpf-check_report-v1-0-efe9a2a1ed28@kernel.org?part=3
>