[PATCH 0/3] HID: bpf: fix 7.3-rc0 bpf breakage

[PATCH 0/3] HID: bpf: fix 7.3-rc0 bpf breakage

[Benjamin Tissoires]

With the introduction of commit ee9ad135b208 ("bpf: Reject a store
through a fault prone pointer"), a HID-BPF program which would try to
change the name, phys or uniq of a HID device would be rejected by the
verifier. Also, this commits and its siblings manage to detect the
buffer overflow that is currently dynamically detected in the test
suite. The verifier now rejects the bpf program we try to load, marking
the test as fail. This is correct behavior, and better admittedly so fix
this so the CI stays green.

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (3):
      HID: bpf: mark struct hid_device as safe BPF pointer
      selftests/hid: Add a test to ensure we can write fields in hid_device
      selftests/hid: prepare test_rdesc_fixup_get_data_overflow for the new verifier

 drivers/hid/bpf/hid_bpf_struct_ops.c               |  6 +++
 tools/testing/selftests/hid/hid_bpf.c              | 51 ++++++++++++++++++----
 tools/testing/selftests/hid/progs/hid.c            | 26 +++++++++++
 .../testing/selftests/hid/progs/hid_bpf_helpers.h  |  3 ++
 4 files changed, 78 insertions(+), 8 deletions(-)
---
base-commit: a93f3bf4e1d60777b1659b812c9e818cfc53b449
change-id: 20260821-wip-bpf-safe-39c069cd8334

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

[PATCH 1/3] HID: bpf: mark struct hid_device as safe BPF pointer

[Benjamin Tissoires]

Commit ee9ad135b208 ("bpf: Reject a store through a fault prone
pointer") in the BPF tree makes the verifier reject any writes to
hid_device->{name,uniq,phys}. A simple solution is to mark the struct
hid_device as safe from a BPF point of view.

Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 drivers/hid/bpf/hid_bpf_struct_ops.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
index 702c22fae136..56c53aca4511 100644
--- a/drivers/hid/bpf/hid_bpf_struct_ops.c
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -62,6 +62,10 @@ struct hid_bpf_offset_write_range {
 	u32 end;
 };
 
+struct hid_bpf_ctx__safe_trusted {
+	struct hid_device *hid;
+};
+
 static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
 					   const struct bpf_reg_state *reg,
 					   int off, int size)
@@ -86,6 +90,8 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
 	const char *cur = NULL;
 	int i;
 
+	BTF_TYPE_EMIT(struct hid_bpf_ctx__safe_trusted);
+
 	t = btf_type_by_id(reg->btf, reg->btf_id);
 
 	for (i = 0; i < ARRAY_SIZE(write_ranges); i++) {

-- 
2.55.0

[PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device

[Benjamin Tissoires]

hid_device->{name,uniq,phys} are all writeable fields, we need to have
tests for them in case the verifier becomes too much strict.

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 tools/testing/selftests/hid/hid_bpf.c              | 26 ++++++++++++++++++++++
 tools/testing/selftests/hid/progs/hid.c            | 26 ++++++++++++++++++++++
 .../testing/selftests/hid/progs/hid_bpf_helpers.h  |  3 +++
 3 files changed, 55 insertions(+)

diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index b851339308c2..069ebdbb4d1c 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -909,6 +909,32 @@ TEST_F(hid_bpf, test_rdesc_fixup_get_data_overflow)
 	ASSERT_EQ(self->skel->bss->get_data_overflow_check, 1);
 }
 
+TEST_F(hid_bpf, test_rdesc_fixup_change_uniq_name_phys)
+{
+	const struct test_program progs[] = {
+		{ .name = "hid_rdesc_fixup_change_uniq_name_phys" },
+	};
+	char expected[256], buf[256] = {};
+	int err;
+
+	LOAD_PROGRAMS(progs);
+
+	err = ioctl(self->hidraw_fd, HIDIOCGRAWNAME(sizeof(buf)), buf);
+	ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWNAME");
+	ASSERT_STREQ("name coming from bpf", buf);
+
+	snprintf(expected, sizeof(expected), "%d phys:coming:from:bpf", self->hid.dev_id);
+
+	err = ioctl(self->hidraw_fd, HIDIOCGRAWPHYS(sizeof(buf)), buf);
+	ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWPHYS");
+	ASSERT_STREQ(expected, buf);
+
+	err = ioctl(self->hidraw_fd, HIDIOCGRAWUNIQ(sizeof(buf)), buf);
+	ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWUNIQ");
+	ASSERT_STREQ("uniq:coming:from:bpf", buf);
+
+}
+
 static int libbpf_print_fn(enum libbpf_print_level level,
 			   const char *format, va_list args)
 {
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index b21fbb13c926..b5d9aea1bda1 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -255,6 +255,32 @@ struct hid_bpf_ops rdesc_fixup_get_data_overflow = {
 	.hid_rdesc_fixup = (void *)hid_rdesc_fixup_get_data_overflow,
 };
 
+SEC("?struct_ops.s/hid_rdesc_fixup")
+int BPF_PROG(hid_rdesc_fixup_change_uniq_name_phys, struct hid_bpf_ctx *hid_ctx)
+{
+#define HID_BPF_MEMCPY(target, str) \
+	__builtin_memcpy(target, str, sizeof(str))
+
+	HID_BPF_MEMCPY(hid_ctx->hid->name, "name coming from bpf");
+	HID_BPF_MEMCPY(hid_ctx->hid->uniq, "uniq:coming:from:bpf");
+	/* hid_bpf relies on a phys being a rand % 1024 */
+	for (int i = 0; i < 4; i++) {
+		if (!hid_ctx->hid->phys[i]) {
+			HID_BPF_MEMCPY(hid_ctx->hid->phys + i, " phys:coming:from:bpf");
+			break;
+		}
+	}
+
+#undef HID_BPF_CONCAT
+
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct hid_bpf_ops rdesc_fixup_change_uniq_name_phys = {
+	.hid_rdesc_fixup = (void *)hid_rdesc_fixup_change_uniq_name_phys,
+};
+
 SEC("?struct_ops/hid_device_event")
 int BPF_PROG(hid_test_insert1, struct hid_bpf_ctx *hid_ctx, enum hid_report_type type)
 {
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index cdca912f3afd..05698793762a 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -61,6 +61,9 @@ enum hid_report_type {
 
 struct hid_device {
 	unsigned int id;
+	char name[128];
+	char phys[64];
+	char uniq[64];
 } __attribute__((preserve_access_index));
 
 struct bpf_wq {

-- 
2.55.0

Re: [PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Flaky test due to off-by-one error when searching for the null terminator of a device ID string.
- [Low] Incorrect macro undefinition leaves `HID_BPF_MEMCPY` defined and attempts to undefine a nonexistent macro.
--

--- Patch [2]: [PATCH 2/3] selftests/hid: Add a test to ensure we can write fields in hid_device ---
commit ac5a75219b6789481c51176040f60dd282ac248f
Author: Benjamin Tissoires <bentiss@kernel.org>

selftests/hid: Add a test to ensure we can write fields in hid_device

This commit adds a selftest to verify that the name, uniq, and phys
fields of struct hid_device can be modified by a BPF program. This
ensures these fields remain writeable for BPF programs.

> diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
> index b851339308c21..069ebdbb4d1c4 100644
> --- a/tools/testing/selftests/hid/hid_bpf.c
> +++ b/tools/testing/selftests/hid/hid_bpf.c
[ ... ]
> +TEST_F(hid_bpf, test_rdesc_fixup_change_uniq_name_phys)
> +{
[ ... ]
> +	snprintf(expected, sizeof(expected), "%d phys:coming:from:bpf", self->hid.dev_id);
> +
> +	err = ioctl(self->hidraw_fd, HIDIOCGRAWPHYS(sizeof(buf)), buf);
> +	ASSERT_GE(err, 0) TH_LOG("HIDIOCGRAWPHYS");
> +	ASSERT_STREQ(expected, buf);

[Severity: Medium]
Will this test intermittently trigger a regression if dev_id is 1000 or
greater?

In setup_uhid(), dev_id is set as a random number between 0 and 1023. If
the number is 4 digits long, the string representation takes 4 characters,
placing the null terminator at index 4, which the BPF loop below doesn't
reach. This would cause the ASSERT_STREQ check to fail because the suffix
is not appended.

[ ... ]
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index b21fbb13c926f..b5d9aea1bda1d 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
[ ... ]
> +SEC("?struct_ops.s/hid_rdesc_fixup")
> +int BPF_PROG(hid_rdesc_fixup_change_uniq_name_phys, struct hid_bpf_ctx *hid_ctx)
> +{
> +#define HID_BPF_MEMCPY(target, str) \
> +	__builtin_memcpy(target, str, sizeof(str))
> +
> +	HID_BPF_MEMCPY(hid_ctx->hid->name, "name coming from bpf");
> +	HID_BPF_MEMCPY(hid_ctx->hid->uniq, "uniq:coming:from:bpf");
> +	/* hid_bpf relies on a phys being a rand % 1024 */
> +	for (int i = 0; i < 4; i++) {
> +		if (!hid_ctx->hid->phys[i]) {
> +			HID_BPF_MEMCPY(hid_ctx->hid->phys + i, " phys:coming:from:bpf");
> +			break;
> +		}
> +	}

[Severity: Medium]
Could this loop condition cause regressions when the phys string is 4
characters long?

If the random dev_id is >= 1000, the string will take up indices 0
through 3, and the null terminator will be at index 4. The loop stops at
i = 3, so it will miss the null terminator and skip the memcpy. Should the
loop condition be i < 5?

> +
> +#undef HID_BPF_CONCAT

[Severity: Low]
This isn't a bug, but it looks like there is a typo here. Should this be
undefining HID_BPF_MEMCPY instead?

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

[PATCH 3/3] selftests/hid: prepare test_rdesc_fixup_get_data_overflow for the new verifier

[Benjamin Tissoires]

The new verifier in the bpf-next branch is now capable of detecting the
overflow that was triggered by test_rdesc_fixup_get_data_overflow.
This is better in terms of UI, but now the test is failing and should be
marked as expected to fail.

Add a new parameter to load_programs() when we expect the test to fail,
and dynamically validate the test by checkcing if it loads (it should
fail to load with new verifier), but if it still loads, HID-BPF should
detect the overflow itself and return an error in hid_bpf_get_data().

Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
 tools/testing/selftests/hid/hid_bpf.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 069ebdbb4d1c..7ab86296ff23 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -67,14 +67,17 @@ struct test_program {
 	int insert_head;
 };
 #define LOAD_PROGRAMS(progs) \
-	load_programs(progs, ARRAY_SIZE(progs), _metadata, self, variant)
+	load_programs(progs, ARRAY_SIZE(progs), false, _metadata, self, variant)
+#define LOAD_PROGRAMS_MAY_FAIL(progs) \
+	load_programs(progs, ARRAY_SIZE(progs), true, _metadata, self, variant)
 #define LOAD_BPF \
-	load_programs(NULL, 0, _metadata, self, variant)
-static void load_programs(const struct test_program programs[],
-			  const size_t progs_count,
-			  struct __test_metadata *_metadata,
-			  FIXTURE_DATA(hid_bpf) * self,
-			  const FIXTURE_VARIANT(hid_bpf) * variant)
+	load_programs(NULL, 0, false, _metadata, self, variant)
+static int load_programs(const struct test_program programs[],
+			 const size_t progs_count,
+			 bool load_may_fail,
+			 struct __test_metadata *_metadata,
+			 FIXTURE_DATA(hid_bpf) * self,
+			 const FIXTURE_VARIANT(hid_bpf) * variant)
 {
 	struct bpf_map *iter_map;
 	int err = -EINVAL;
@@ -128,6 +131,9 @@ static void load_programs(const struct test_program programs[],
 	}
 
 	err = hid__load(self->skel);
+	if (err && load_may_fail)
+		return err;
+
 	ASSERT_OK(err) TH_LOG("hid_skel_load failed: %d", err);
 
 	for (int i = 0; i < progs_count; i++) {
@@ -147,6 +153,7 @@ static void load_programs(const struct test_program programs[],
 
 	self->hidraw_fd = open_hidraw(&self->hid);
 	ASSERT_GE(self->hidraw_fd, 0) TH_LOG("open_hidraw");
+	return 0;
 }
 
 /*
@@ -904,7 +911,9 @@ TEST_F(hid_bpf, test_rdesc_fixup_get_data_overflow)
 		{ .name = "hid_rdesc_fixup_get_data_overflow" },
 	};
 
-	LOAD_PROGRAMS(progs);
+	/* newer verifier can detect the overflow at load time */
+	if (LOAD_PROGRAMS_MAY_FAIL(progs))
+		return;
 
 	ASSERT_EQ(self->skel->bss->get_data_overflow_check, 1);
 }

-- 
2.55.0