[PATCH 0/2] HID: bpf: close device teardown races

[PATCH 0/2] HID: bpf: close device teardown races

[Yifan Zhang]

HID-BPF can race device destruction in two paths.  A hardware request can
outlive transport-private state, and a struct_ops registration can resume
after destruction has cleaned the device's SRCU state.

Serialize HID-BPF hardware operations with the destruction transition, then
reject registrations that acquire prog_list_lock after that transition.

Yifan Zhang (2):
  HID: bpf: serialize hardware operations with device destruction
  HID: bpf: reject struct_ops registration during device destruction

 drivers/hid/bpf/hid_bpf_dispatch.c   | 36 +++++++++++++++++++---------
 drivers/hid/bpf/hid_bpf_struct_ops.c |  5 ++++
 2 files changed, 30 insertions(+), 11 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH 1/2] HID: bpf: serialize hardware operations with device destruction

[Yifan Zhang]

A HID-BPF syscall context pins struct hid_device, but it does not pin the
transport-private data referenced by the low-level driver.  A hardware
request can therefore sleep in the UHID report-wait path while closing the
UHID file destroys the HID device, wakes the waiter, and frees struct
uhid_device.  The awakened request then accesses the freed object.

KASAN reports:

  BUG: KASAN: use-after-free in __uhid_report_queue_and_wait
  Read of size 1
  uhid_hid_raw_request
  __hid_hw_raw_request
  hid_bpf_hw_request
  bpf_prog_test_run_syscall

Hold prog_list_lock across HID-BPF hardware operations.  Device destruction
sets destroyed while holding the same lock, so it waits for an operation
that has already started.  Later operations return -ENODEV before reaching
the low-level driver.  Apply the same lifetime rule to output reports.

The reproducer completed 100 close-versus-request trials without a KASAN
report with this change.

Fixes: 91a7f802d185 ("HID: bpf: introduce hid_hw_request()")
Closes: https://lore.kernel.org/linux-input/179317460423.87432.1451672332362541157@stu.pku.edu.cn/
Signed-off-by: Yifan Zhang <yfzhang23@stu.pku.edu.cn>
---
 drivers/hid/bpf/hid_bpf_dispatch.c | 36 +++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

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);
 
 	kfree(dma_data);
 	return ret;
@@ -641,8 +653,10 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
 	if (!hdev)
 		return;
 
-	/* mark the device as destroyed in bpf so we don't reattach it */
-	hdev->bpf.destroyed = true;
+	/* Block new BPF hardware operations before transport teardown. */
+	mutex_lock(&hdev->bpf.prog_list_lock);
+	WRITE_ONCE(hdev->bpf.destroyed, true);
+	mutex_unlock(&hdev->bpf.prog_list_lock);
 
 	__hid_bpf_ops_destroy_device(hdev);
 
-- 
2.39.5 (Apple Git-154)

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

[Yifan Zhang]

hid_bpf_reg() can obtain a device reference before hid_destroy_device()
removes the device from the bus, then block on prog_list_lock while HID-BPF
destruction cleans the device's SRCU state.  The registration path does not
check the destroyed flag after taking the lock, so it can publish a new
struct_ops entry and call synchronize_srcu() on the cleaned SRCU object.

KASAN reports:

  BUG: KASAN: slab-use-after-free in check_init_srcu_struct
  Read of size 8
  check_init_srcu_struct
  synchronize_srcu
  hid_bpf_reg
  bpf_struct_ops_link_create

Check destroyed while holding prog_list_lock.  The preceding teardown
serialization change sets the flag while holding the same lock, so either a
registration completes before destruction starts or it fails with -ENODEV
without accessing SRCU.

The reproducer no longer triggers a KASAN report with this change.

Fixes: ebc0d8093e8c ("HID: bpf: implement HID-BPF through bpf_struct_ops")
Closes: https://lore.kernel.org/linux-input/179317460937.87455.1799017093083467190@stu.pku.edu.cn/
Signed-off-by: Yifan Zhang <yfzhang23@stu.pku.edu.cn>
---
 drivers/hid/bpf/hid_bpf_struct_ops.c | 5 +++++
 1 file changed, 5 insertions(+)

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;
+	}
+
 	count = list_count_nodes(&hdev->bpf.prog_list);
 	if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
 		err = -E2BIG;
-- 
2.39.5 (Apple Git-154)

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

[sashiko-bot]

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

Re: [PATCH 1/2] HID: bpf: serialize hardware operations with device destruction

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] HID: bpf: report UAF in __hid_bpf_hw_check_params() during device teardown
- [High] HID: bpf: ABBA deadlock between prog_list_lock and srcu in hw request kfuncs
- [High] HID: bpf: recursive prog_list_lock deadlock in hid_bpf_hw_request()
- [High] HID: bpf: lock inversion between prog_list_lock and driver_input_lock
--

commit 49a737ce63da80cb915d4530a8a28ce52af759cc
Author: Yifan Zhang <yfzhang23@stu.pku.edu.cn>

HID: bpf: serialize hardware operations with device destruction

This commit serializes HID-BPF hardware requests and output reports with
device destruction by holding prog_list_lock and checking the destroyed
flag across low-level driver calls in hid_bpf_hw_request() and
hid_bpf_hw_output_report().

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

[Severity: High]
Can acquiring ctx->hid->bpf.prog_list_lock here in hid_bpf_hw_request()
(and below in hid_bpf_hw_output_report()) in
drivers/hid/bpf/hid_bpf_dispatch.c cause an ABBA deadlock with
hdev->bpf.srcu?

In drivers/hid/bpf/hid_bpf_struct_ops.c, both hid_bpf_reg() and
hid_bpf_unreg() hold prog_list_lock while waiting on synchronize_srcu():

drivers/hid/bpf/hid_bpf_struct_ops.c:hid_bpf_unreg() {
	...
	mutex_lock(&hdev->bpf.prog_list_lock);
	...
	list_del_rcu(&ops->list);
	synchronize_srcu(&hdev->bpf.srcu);
	...
	mutex_unlock(&hdev->bpf.prog_list_lock);
}

When hid_bpf_hw_request() or hid_bpf_hw_output_report() is invoked from a
syscall BPF program, it also takes prog_list_lock and then calls
__hid_hw_raw_request() or __hid_hw_output_report(), which enters
dispatch_hid_bpf_raw_requests() or dispatch_hid_bpf_output_report() and
takes srcu_read_lock(&hdev->bpf.srcu). Both paths establish the lock order
prog_list_lock -> hdev->bpf.srcu.

However, hid_bpf_hw_request() and hid_bpf_hw_output_report() are also
registered in hid_bpf_kfunc_ids for BPF_PROG_TYPE_STRUCT_OPS, so sleepable
struct_ops callbacks (e->hid_hw_request and e->hid_hw_output_report) can
call them directly. When a non-BPF caller such as hidraw invokes
__hid_hw_raw_request(..., from_bpf=false) or
__hid_hw_output_report(..., from_bpf=false),
dispatch_hid_bpf_raw_requests() or dispatch_hid_bpf_output_report() holds
srcu_read_lock(&hdev->bpf.srcu) across the callback:

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);
		if (ret)
			goto out;
	}
	ret = 0;

out:
	srcu_read_unlock(&hdev->bpf.srcu, idx);
	return ret;
}

When that struct_ops callback calls hid_bpf_hw_request() or
hid_bpf_hw_output_report(), it acquires prog_list_lock while holding
srcu_read_lock(&hdev->bpf.srcu), inverting the lock order to
hdev->bpf.srcu -> prog_list_lock and deadlocking if another thread is in
hid_bpf_reg() or hid_bpf_unreg().

[Severity: High]
Could holding ctx->hid->bpf.prog_list_lock across
hid_ops->hid_hw_raw_request() in hid_bpf_hw_request() (and across
hid_ops->hid_hw_output_report() in hid_bpf_hw_output_report()) also lead
to a recursive mutex self-deadlock on the same task?

When hid_ops->hid_hw_raw_request(..., true) or
hid_ops->hid_hw_output_report(..., true) runs with prog_list_lock held,
__hid_hw_raw_request() and __hid_hw_output_report() still invoke
dispatch_hid_bpf_raw_requests() and dispatch_hid_bpf_output_report(),
which call any attached sleepable struct_ops e->hid_hw_request or
e->hid_hw_output_report programs on that hid_device.

The recursion check at the start of hid_bpf_hw_request() and
hid_bpf_hw_output_report() only inspects ctx_kern->from_bpf on the passed
context:

drivers/hid/bpf/hid_bpf_dispatch.c:hid_bpf_hw_request() {
	ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);

	if (ctx_kern->from_bpf)
		return -EDEADLOCK;
	...
}

Because hid_bpf_allocate_context() is exposed in hid_bpf_kfunc_ids to
sleepable struct_ops programs and returns a fresh context with
from_bpf == false, wouldn't a struct_ops program that calls
hid_bpf_allocate_context(hctx->hid->id) and passes the new context to
hid_bpf_hw_request() or hid_bpf_hw_output_report() bypass the
ctx_kern->from_bpf check and try to lock ctx->hid->bpf.prog_list_lock a
second time?

[Severity: High]
Does holding ctx->hid->bpf.prog_list_lock across
hid_ops->hid_hw_raw_request() and hid_ops->hid_hw_output_report() in
drivers/hid/bpf/hid_bpf_dispatch.c introduce a lock inversion with
hdev->driver_input_lock?

In drivers/hid/hid-core.c, hid_device_probe() and hid_device_remove()
acquire hdev->driver_input_lock before calling __hid_device_probe() (which
runs call_hid_bpf_rdesc_fixup() and hdrv->probe()) and hdrv->remove(). If
a sleepable struct_ops callback (such as hid_rdesc_fixup, hid_hw_request,
or hid_hw_output_report) invoked under driver_input_lock calls
hid_bpf_hw_request() or hid_bpf_hw_output_report(), it acquires
prog_list_lock while holding driver_input_lock
(driver_input_lock -> prog_list_lock).

Conversely, while hid_bpf_hw_request() or hid_bpf_hw_output_report() holds
prog_list_lock across hid_ops->hid_hw_raw_request() or
hid_ops->hid_hw_output_report(), an attached sleepable struct_ops callback
e->hid_hw_request or e->hid_hw_output_report can call
hid_bpf_input_report(), which is exposed in hid_bpf_kfunc_ids and acquires
driver_input_lock before checking ctx_kern->from_bpf:

drivers/hid/bpf/hid_bpf_dispatch.c:hid_bpf_input_report() {
	...
	ret = down_interruptible(&ctx->hid->driver_input_lock);
	if (ret)
		return ret;
	...
}

Wouldn't that establish the reverse ordering
prog_list_lock -> driver_input_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);
>  
>  	kfree(dma_data);
>  	return ret;
> @@ -641,8 +653,10 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
>  	if (!hdev)
>  		return;
>  
> -	/* mark the device as destroyed in bpf so we don't reattach it */
> -	hdev->bpf.destroyed = true;
> +	/* Block new BPF hardware operations before transport teardown. */
> +	mutex_lock(&hdev->bpf.prog_list_lock);
> +	WRITE_ONCE(hdev->bpf.destroyed, true);
> +	mutex_unlock(&hdev->bpf.prog_list_lock);

[Severity: Critical]
Is checking ctx->hid->bpf.destroyed after __hid_bpf_hw_check_params() in
hid_bpf_hw_request() and hid_bpf_hw_output_report() sufficient to prevent
a use-after-free on struct hid_report during device teardown?

In drivers/hid/bpf/hid_bpf_dispatch.c, both hid_bpf_hw_request() and
hid_bpf_hw_output_report() call __hid_bpf_hw_check_params() before
acquiring ctx->hid->bpf.prog_list_lock and before checking
ctx->hid->bpf.destroyed:

drivers/hid/bpf/hid_bpf_dispatch.c:hid_bpf_hw_request() {
	...
	/* check arguments */
	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
	if (ret)
		return ret;
	...
	mutex_lock(&ctx->hid->bpf.prog_list_lock);
	if (READ_ONCE(ctx->hid->bpf.destroyed)) {
	...
}

__hid_bpf_hw_check_params() looks up and dereferences struct hid_report
from ctx->hid->report_enum:

drivers/hid/bpf/hid_bpf_dispatch.c:__hid_bpf_hw_check_params() {
	...
	report_enum = ctx->hid->report_enum + rtype;
	report = hid_ops->hid_get_report(report_enum, buf);
	if (!report)
		return -EINVAL;

	report_len = hid_report_len(report);
	...
}

During hid_destroy_device() in drivers/hid/hid-core.c,
hid_bpf_destroy_device() sets hdev->bpf.destroyed = true and unlocks
prog_list_lock, and then hid_remove_device() -> device_del() ->
hid_device_remove() -> hid_close_report() frees each report in
report_enum->report_id_hash[] before zeroing report_enum:

drivers/hid/hid-core.c:hid_close_report() {
	...
	for (i = 0; i < HID_REPORT_TYPES; i++) {
		struct hid_report_enum *report_enum = device->report_enum + i;

		for (j = 0; j < HID_MAX_IDS; j++) {
			struct hid_report *report = report_enum->report_id_hash[j];
			if (report)
				hid_free_report(report);
		}
		memset(report_enum, 0, sizeof(*report_enum));
	}
}

Because a syscall BPF context allocated by hid_bpf_allocate_context()
only pins struct hid_device via get_device(&hdev->dev), can a concurrent
call to hid_bpf_hw_request() or hid_bpf_hw_output_report() race with
hid_close_report() inside __hid_bpf_hw_check_params() and dereference a
freed struct hid_report before reaching the prog_list_lock and destroyed
check?

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

Re: [PATCH 1/2] HID: bpf: serialize hardware operations with device destruction

[Alexei Starovoitov]

On Mon, Nov 02, 2026 at 03:58 AM Yifan Zhang <yfzhang23@stu.pku.edu.cn> wrote:
> Hold prog_list_lock across HID-BPF hardware operations.  Device destruction
> sets destroyed while holding the same lock, so it waits for an operation
> that has already started.  Later operations return -ENODEV before reaching
> the low-level driver.  Apply the same lifetime rule to output reports.

sashiko is right. This will deadlock.

pw-bot: cr