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

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 <[email protected]>
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/[email protected]?part=1