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

[sashiko-bot@kernel.org]

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