Re: [PATCH v2 2/2] HID: bpf: reject struct_ops registration during device destruction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] HID: bpf: premature ops->hdev assignment before lock in hid_bpf_reg()
--

commit 652bc4fabe060de525612119c444207ef100747a
Author: Yifan Zhang <yfzhang23@stu.pku.edu.cn>

HID: bpf: reject struct_ops registration during device destruction

This commit checks hdev->bpf.destroyed while holding prog_list_lock in
hid_bpf_reg() to avoid registering a struct_ops program and calling
synchronize_srcu() after HID-BPF device destruction has cleaned up the
device's SRCU state. It also clears ops->hdev under prog_list_lock on
registration failure before dropping the device reference.

> diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> index c90b68956cb3b..31e2cb146b92c 100644
> --- a/drivers/hid/bpf/hid_bpf_struct_ops.c
> +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> @@ -201,6 +201,11 @@ static int hid_bpf_reg(void *kdata, struct bpf_link *link)
>  
>  	mutex_lock(&hdev->bpf.prog_list_lock);
>  
> +	if (READ_ONCE(hdev->bpf.destroyed)) {
> +		err = -ENODEV;
> +		goto out_unlock;
> +	}
> +
>  	count = list_count_nodes(&hdev->bpf.prog_list);
>  	if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
>  		err = -E2BIG;
> @@ -229,11 +234,14 @@ static int hid_bpf_reg(void *kdata, struct bpf_link *link)
>  	synchronize_srcu(&hdev->bpf.srcu);
>  
>  out_unlock:
> -	mutex_unlock(&hdev->bpf.prog_list_lock);
> -
>  	if (err) {
>  		if (hdev->bpf.rdesc_ops == ops)
>  			hdev->bpf.rdesc_ops = NULL;
> +		ops->hdev = NULL;
> +	}
> +	mutex_unlock(&hdev->bpf.prog_list_lock);
> +
> +	if (err) {
>  		hid_put_device(hdev);

[Severity: Critical]
In drivers/hid/bpf/hid_bpf_struct_ops.c, hid_bpf_reg() still assigns
ops->hdev = hdev before acquiring hdev->bpf.prog_list_lock:

drivers/hid/bpf/hid_bpf_struct_ops.c:hid_bpf_reg() {
	...
	hdev = hid_get_device(ops->hid_id);
	if (IS_ERR(hdev))
		return PTR_ERR(hdev);

	ops->hdev = hdev;

	mutex_lock(&hdev->bpf.prog_list_lock);
}

When a HID device with an attached struct_ops link is destroyed in
hid_destroy_device() -> hid_bpf_destroy_device(),
__hid_bpf_ops_destroy_device() clears ops->hdev = NULL under prog_list_lock
and drops the link's device reference while leaving the existing bpf_link
alive, and hid_bpf_destroy_device() then calls
cleanup_srcu_struct(&hdev->bpf.srcu). Because hid_remove_device() has not
called device_del(&hdev->dev) yet, hid_get_device() can still find hdev on
hid_bus_type.

If a concurrent bpf_struct_ops_link_create() on the same struct_ops map calls
hid_bpf_reg(), it sees ops->hdev == NULL and writes ops->hdev = hdev before
taking prog_list_lock. If the original struct_ops link is closed at the same
time, bpf_struct_ops_map_link_dealloc() -> hid_bpf_unreg() runs without
holding the map's update_mutex:

drivers/hid/bpf/hid_bpf_struct_ops.c:hid_bpf_unreg() {
	...
	hdev = ops->hdev;

	/* check if __hid_bpf_ops_destroy_device() has been called */
	if (!hdev)
		return;

	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;
	...
	hid_put_device(hdev);
}

Can hid_bpf_unreg() observe the newly written ops->hdev, acquire
prog_list_lock before hid_bpf_reg(), bypass the !ops->hdev check, and call
synchronize_srcu(&hdev->bpf.srcu) on the cleaned-up srcu_struct followed by a
duplicate hid_put_device(hdev) when hid_bpf_reg() also drops its reference in
the error path?

Should ops->hdev only be checked and assigned after acquiring
hdev->bpf.prog_list_lock and verifying that hdev->bpf.destroyed is false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179367391951.8187.14164894863012056170@stu.pku.edu.cn?part=2