[PATCH] HID: bpf: serialize device reference release in struct_ops destroy path

[PATCH] HID: bpf: serialize device reference release in struct_ops destroy path

[Shen Yongchao]

__hid_bpf_ops_destroy_device() and hid_bpf_unreg() can race on the
same registration reference, double-putting struct hid_device and
freeing it while hid_destroy_device() still uses it.  Serialize the
remove/NULL decision under hdev->bpf.prog_list_lock so exactly one
path releases each registration reference: unreg re-checks ops->hdev
under the lock and returns without putting when the destroy path
already cleared it; all put_device() calls happen after the lock is
dropped, which is safe because a concurrent unreg then observes
ops->hdev == NULL under the lock.

Background: each successful attach (hid_bpf_ops_reg) acquires one
device reference (hid_get_device()).  Two paths can release it:

- device destruction: hid_destroy_device() -> hid_bpf_destroy_device()
  -> __hid_bpf_ops_destroy_device(), which walks hdev->bpf.prog_list
  under rcu_read_lock() and drops one reference per attached program;
- BPF link release: bpf map delete (no BPF_F_LINK) synchronously calls
  st_ops->unreg() -> hid_bpf_unreg(), which drops the reference for
  its own registration.

The coordination handshake (e->hdev = NULL on the destroy side vs
"if (!hdev) return" on the unreg side) is a TOCTOU check: the two
paths run under different lock domains (rcu_read_lock vs
prog_list_lock), so a concurrent unreg can read ops->hdev as
non-NULL, block on prog_list_lock, and then proceed while the
destroy traversal executes - both paths then drop the same
reference.  The refcount reaches zero legitimately (each decrement
is individually valid), so no refcount_t saturation fires: the
device is simply freed while the transport is still inside
hid_destroy_device(), and subsequent teardown touches freed memory.

The fix serializes the remove/NULL decision under prog_list_lock on
both sides and moves the destroy-side puts outside the lock.  With
the lock held, plain reads/writes of ops->hdev are sufficient; no
READ_ONCE/WRITE_ONCE are added, keeping the patch minimal.

Unlocked-read safety: the unlocked read of ops->hdev at the top of
hid_bpf_unreg() cannot touch a freed device, because the unreg path
itself still holds this registration's reference (released only by
its own hid_put_device() after the lock is dropped), and a destroy
traversal that already cleared ops->hdev makes the lock-internal
re-check return early without any put.  At most one of the two
paths releases each registration reference.

Fixes: ebc0d8093e8c ("HID: bpf: implement HID-BPF through bpf_struct_ops")
Cc: stable@vger.kernel.org
Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Assisted-by: Hermes:kimi-k3
---
Testing:

- KASAN on v7.1.4 (CONFIG_KASAN=y, CONFIG_HID_BPF=y, CONFIG_UHID=y):
  the unfixed kernel reports slab-use-after-free in hid_disconnect()
  (993 reports across runs); a 3-thread PoC (two uhid devices +
  struct_ops programs attached without BPF_F_LINK + concurrent
  bpf_map_delete_elem() and write(UHID_DESTROY)) triggers on the
  first iteration.
- With this fix: 400/400 iterations, 0 KASAN reports, PoC exit 0.
- Control: a partial fix that only reorders the NULL/put (without
  serializing under the lock) still triggers 993 KASAN reports,
  confirming the full serialization is required.
- kprobe interleaving shows unreg and destroy about 5us apart on
  different CPUs.
- PoC source (164 lines) available on request.

Alternative considered: an atomic-exchange (xchg) ownership variant
was drafted but not tested; the lock-based serialization above is
preferred (uses the subsystem's existing prog_list_lock) and is the
only one with dynamic verification.

 drivers/hid/bpf/hid_bpf_struct_ops.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
index 702c22fae13..049df064776 100644
--- a/drivers/hid/bpf/hid_bpf_struct_ops.c
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -250,6 +250,11 @@ static void hid_bpf_unreg(void *kdata, struct bpf_link *link)
 
 	mutex_lock(&hdev->bpf.prog_list_lock);
 
+	if (!ops->hdev) {
+		mutex_unlock(&hdev->bpf.prog_list_lock);
+		return;
+	}
+
 	list_del_rcu(&ops->list);
 	synchronize_srcu(&hdev->bpf.srcu);
 	ops->hdev = NULL;
@@ -310,13 +315,17 @@ static struct bpf_struct_ops bpf_hid_bpf_ops = {
 void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
 {
 	struct hid_bpf_ops *e;
+	int count = 0;
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
-		hid_put_device(hdev);
+	mutex_lock(&hdev->bpf.prog_list_lock);
+	list_for_each_entry(e, &hdev->bpf.prog_list, list) {
 		e->hdev = NULL;
+		count++;
 	}
-	rcu_read_unlock();
+	mutex_unlock(&hdev->bpf.prog_list_lock);
+
+	while (count--)
+		hid_put_device(hdev);
 }
 
 static int __init hid_bpf_struct_ops_init(void)
-- 

Re: [PATCH] HID: bpf: serialize device reference release in struct_ops destroy path

[sashiko-bot]

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

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race leading to a Use-After-Free of `hdev` during lock acquisition.
- [High] Early return bypasses `list_del_rcu()` and `synchronize_srcu()`, leaving a freed BPF ops object in the device's active program list.
--

--- Patch [1]: [PATCH] HID: bpf: serialize device reference release in struct_ops destroy path ---
commit cfdd4ce27d8a975674b1f3418e6b5078b5d94946
Author: Shen Yongchao <grayhat@foxmail.com>
Subject: HID: bpf: serialize device reference release in struct_ops destroy path

This patch aims to fix a double-put race condition in the HID BPF struct_ops
destroy and unreg paths. It serializes the remove/NULL decision under
hdev->bpf.prog_list_lock to ensure exactly one path releases the device
reference.

> diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> index 702c22fae136a..049df06477610 100644
> --- a/drivers/hid/bpf/hid_bpf_struct_ops.c
> +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> @@ -250,6 +250,11 @@ static void hid_bpf_unreg(void *kdata, struct bpf_link *link)
>  
>  	mutex_lock(&hdev->bpf.prog_list_lock);

[Severity: High]
This is a pre-existing issue, but is it possible for hdev to be completely
freed between being read from ops->hdev and used here in mutex_lock()?

If hid_bpf_unreg() reads hdev = ops->hdev and is then preempted, could
__hid_bpf_ops_destroy_device() execute concurrently, clear ops->hdev, and
drop the registration's device reference via hid_put_device(hdev)?

If that was the final reference, the device could be freed, leading to a
use-after-free when the unreg thread resumes and attempts to lock
&hdev->bpf.prog_list_lock.

> +	if (!ops->hdev) {
> +		mutex_unlock(&hdev->bpf.prog_list_lock);
> +		return;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this early return safely handle list
cleanup?

In __hid_bpf_ops_destroy_device(), e->hdev = NULL is set, but the node is
intentionally left linked in hdev->bpf.prog_list.

If hid_bpf_unreg() runs, sees !ops->hdev under the lock, and returns early
here, it entirely bypasses list_del_rcu(&ops->list) and
synchronize_srcu(&hdev->bpf.srcu).

Since the BPF framework will subsequently free the ops map memory, wouldn't
this leave a freed object in hdev->bpf.prog_list?

If a driver .remove callback then triggers hid_bpf_hw_request() (for example,
to turn off LEDs), it would traverse the list under SRCU and access the freed
ops object.

>  	list_del_rcu(&ops->list);

[ ... ]

> @@ -310,13 +315,17 @@ static struct bpf_struct_ops bpf_hid_bpf_ops = {
>  void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
>  {
>  	struct hid_bpf_ops *e;
> +	int count = 0;
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> -		hid_put_device(hdev);
> +	mutex_lock(&hdev->bpf.prog_list_lock);
> +	list_for_each_entry(e, &hdev->bpf.prog_list, list) {
>  		e->hdev = NULL;
> +		count++;
>  	}
> -	rcu_read_unlock();
> +	mutex_unlock(&hdev->bpf.prog_list_lock);
> +
> +	while (count--)
> +		hid_put_device(hdev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_6C4272FCE4F12C891C300445A9D8E6C68C0A@qq.com?part=1