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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] HID: bpf: dangling ops->hdev pointer on error path in hid_bpf_reg()
- [High] HID: bpf: deadlock on prog_list_lock and srcu in hid_bpf_hw_request()
--

commit 318c71f825ad280448e4c11859ac62903f968c18
Author: Yifan Zhang <yfzhang23@stu.pku.edu.cn>

HID: bpf: reject struct_ops registration during device destruction

This series serializes HID-BPF hardware requests and output reports against
device teardown using prog_list_lock and the destroyed flag, and rejects new
struct_ops registrations once device destruction has started.

> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 6b467c74601cb..4fea42dd2f392 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -444,14 +444,20 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
>  	if (!dma_data)
>  		return -ENOMEM;
>  
> -	ret = hid_ops->hid_hw_raw_request(ctx->hid,
> -					      dma_data[0],
> -					      dma_data,
> -					      size,
> -					      rtype,
> -					      reqtype,
> -					      (u64)(long)ctx,
> -					      true); /* prevent infinite recursions */
> +	mutex_lock(&ctx->hid->bpf.prog_list_lock);
> +	if (READ_ONCE(ctx->hid->bpf.destroyed)) {
> +		ret = -ENODEV;
> +	} else {
> +		ret = hid_ops->hid_hw_raw_request(ctx->hid,
> +						  dma_data[0],
> +						  dma_data,
> +						  size,
> +						  rtype,
> +						  reqtype,
> +						  (u64)(long)ctx,
> +						  true); /* prevent infinite recursions */
> +	}
> +	mutex_unlock(&ctx->hid->bpf.prog_list_lock);
>  
>  	if (ret > (ssize_t)size)
>  		ret = size;
> @@ -492,7 +498,13 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
>  	if (!dma_data)
>  		return -ENOMEM;
>  
> -	ret = hid_ops->hid_hw_output_report(ctx->hid, dma_data, size, (u64)(long)ctx, true);
> +	mutex_lock(&ctx->hid->bpf.prog_list_lock);
> +	if (READ_ONCE(ctx->hid->bpf.destroyed))
> +		ret = -ENODEV;
> +	else
> +		ret = hid_ops->hid_hw_output_report(ctx->hid, dma_data, size,
> +						    (u64)(long)ctx, true);
> +	mutex_unlock(&ctx->hid->bpf.prog_list_lock);

[Severity: High]
Can taking prog_list_lock in hid_bpf_hw_request() and
hid_bpf_hw_output_report() deadlock against hdev->bpf.srcu or against itself?

In drivers/hid/bpf/hid_bpf_struct_ops.c, hid_bpf_reg() and hid_bpf_unreg()
acquire prog_list_lock and then wait in synchronize_srcu():

drivers/hid/bpf/hid_bpf_struct_ops.c:hid_bpf_reg() {
	mutex_lock(&hdev->bpf.prog_list_lock);
	...
	synchronize_srcu(&hdev->bpf.srcu);
}

Meanwhile, in drivers/hid/bpf/hid_bpf_dispatch.c,
dispatch_hid_bpf_raw_requests() and dispatch_hid_bpf_output_report() hold
srcu_read_lock(&hdev->bpf.srcu) while invoking sleepable struct_ops programs
(e->hid_hw_request and e->hid_hw_output_report):

drivers/hid/bpf/hid_bpf_dispatch.c:dispatch_hid_bpf_raw_requests() {
	idx = srcu_read_lock(&hdev->bpf.srcu);
	list_for_each_entry_srcu(e, &hdev->bpf.prog_list, list,
				 srcu_read_lock_held(&hdev->bpf.srcu)) {
		if (!e->hid_hw_request)
			continue;

		ret = e->hid_hw_request(&ctx_kern.ctx, reportnum, rtype, reqtype, source);
		...
	}
	srcu_read_unlock(&hdev->bpf.srcu, idx);
}

Those sleepable callbacks are permitted to call the kfuncs
hid_bpf_hw_request() and hid_bpf_hw_output_report() (as done in
tools/testing/selftests/hid/progs/hid.c). With prog_list_lock taken inside
those kfuncs, a task running in dispatch_hid_bpf_raw_requests() or
dispatch_hid_bpf_output_report() will lock srcu -> prog_list_lock, while a
concurrent hid_bpf_reg() or hid_bpf_unreg() locks prog_list_lock -> srcu,
causing an ABBA deadlock.

Additionally, because prog_list_lock is held across
hid_ops->hid_hw_raw_request() and hid_ops->hid_hw_output_report(), which invoke
dispatch_hid_bpf_raw_requests() and dispatch_hid_bpf_output_report() on the
same task, if an attached sleepable struct_ops program allocates a fresh
context via hid_bpf_allocate_context() (where from_bpf is false) and calls
hid_bpf_hw_request() or hid_bpf_hw_output_report() on the same device, the task
will attempt to acquire prog_list_lock a second time and self-deadlock.

[ ... ]

> diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> index c90b68956cb3b..4c433f419c85e 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;
> +	}

[Severity: Critical]
Does jumping to out_unlock on error in hid_bpf_reg() leave a dangling pointer
in ops->hdev after hid_put_device(hdev) is called?

In drivers/hid/bpf/hid_bpf_struct_ops.c, hid_bpf_reg() assigns ops->hdev = hdev
before acquiring prog_list_lock, and the error path under out_unlock drops the
hdev reference without clearing ops->hdev:

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);

	if (READ_ONCE(hdev->bpf.destroyed)) {
		err = -ENODEV;
		goto out_unlock;
	}
	...
out_unlock:
	mutex_unlock(&hdev->bpf.prog_list_lock);

	if (err) {
		if (hdev->bpf.rdesc_ops == ops)
			hdev->bpf.rdesc_ops = NULL;
		hid_put_device(hdev);
	}
}

When bpf_struct_ops_link_create() fails in st_ops->reg(), the struct_ops map's
kdata is not reset, so leaving ops->hdev non-NULL causes any subsequent attach
attempt on the map to fail at if (ops->hdev) return -EINVAL.

More importantly, if the map is already attached via an active bpf_link while
hid_destroy_device(hdev) runs, hid_bpf_destroy_device() calls
__hid_bpf_ops_destroy_device(hdev) (which clears ops->hdev = NULL and drops the
active link's hdev reference) before hid_remove_device(hdev) removes the device
from hid_bus_type. If a second bpf_struct_ops_link_create() on the same map
races in this window, it passes if (ops->hdev), gets a temporary reference via
hid_get_device(), sets ops->hdev = hdev, hits the new hdev->bpf.destroyed
check, and drops the temporary reference via hid_put_device(hdev) while leaving
ops->hdev pointing to hdev. Once hid_destroy_device() frees hdev, closing the
first bpf_link calls hid_bpf_unreg(), which sees ops->hdev != NULL and
dereferences the freed hdev.

Should ops->hdev be cleared in the if (err) block (or assigned only after the
error checks succeed)?

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