linux-kernel - Re: [PATCH] HID: bpf: abort dispatch if device destroyed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250512211708.35a0d8df@meshulam.tesarici.cz>
Date: Mon, 12 May 2025 21:17:08 +0200
From: Petr Tesařík <petr@...arici.cz>
To: Rong Zhang <i@...g.moe>
Cc: Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 bpf@...r.kernel.org, Brian Gerst <brgerst@...il.com>, Borislav Petkov
 <bp@...en8.de>, bugzilla-daemon@...nel.org, stable@...r.kernel.org
Subject: Re: [PATCH] HID: bpf: abort dispatch if device destroyed

On Mon, 12 May 2025 23:24:19 +0800
Rong Zhang <i@...g.moe> wrote:

> The current HID bpf implementation assumes no output report/request will
> go through it after hid_bpf_destroy_device() has been called. This leads
> to a bug that unplugging certain types of HID devices causes a cleaned-
> up SRCU to be accessed. The bug was previously a hidden failure until a
> recent x86 percpu change [1] made it access not-present pages.
> 
> The bug will be triggered if the conditions below are met:
> 
> A) a device under the driver has some LEDs on
> B) hid_ll_driver->request() is uninplemented (e.g., logitech-djreceiver)
> 
> If condition A is met, hidinput_led_worker() is always scheduled *after*
> hid_bpf_destroy_device().
> 
> hid_destroy_device
> ` hid_bpf_destroy_device
>   ` cleanup_srcu_struct(&hdev->bpf.srcu)
> ` hid_remove_device
>   ` ...
>     ` led_classdev_unregister
>       ` led_trigger_set(led_cdev, NULL)
>         ` led_set_brightness(led_cdev, LED_OFF)
>           ` ...
>             ` input_inject_event
>               ` input_event_dispose
>                 ` hidinput_input_event
>                   ` schedule_work(&hid->led_work) [hidinput_led_worker]
> 
> This is fine when condition B is not met, where hidinput_led_worker()
> calls hid_ll_driver->request(). This is the case for most HID drivers,
> which implement it or use the generic one from usbhid. The driver itself
> or an underlying driver will then abort processing the request.
> 
> Otherwise, hidinput_led_worker() tries hid_hw_output_report() and leads
> to the bug.
> 
> hidinput_led_worker
> ` hid_hw_output_report
>   ` dispatch_hid_bpf_output_report
>     ` srcu_read_lock(&hdev->bpf.srcu)
>     ` srcu_read_unlock(&hdev->bpf.srcu, idx)
> 
> The bug has existed since the introduction [2] of
> dispatch_hid_bpf_output_report(). However, the same bug also exists in
> dispatch_hid_bpf_raw_requests(), and I've reproduced (no visible effect
> because of the lack of [1], but confirmed bpf.destroyed == 1) the bug
> against the commit (i.e., the Fixes:) introducing the function. This is
> because hidinput_led_worker() falls back to hid_hw_raw_request() when
> hid_ll_driver->output_report() is uninplemented (e.g., logitech-
> djreceiver).
> 
> hidinput_led_worker
> ` hid_hw_output_report: -ENOSYS
> ` hid_hw_raw_request
>   ` dispatch_hid_bpf_raw_requests
>     ` srcu_read_lock(&hdev->bpf.srcu)
>     ` srcu_read_unlock(&hdev->bpf.srcu, idx)
> 
> Fix the issue by returning early in the two mentioned functions if
> hid_bpf has been marked as destroyed. Though
> dispatch_hid_bpf_device_event() handles input events, and there is no
> evidence that it may be called after the destruction, the same check, as
> a safety net, is also added to it to maintain the consistency among all
> dispatch functions.
> 
> The impact of the bug on other architectures is unclear. Even if it acts
> as a hidden failure, this is still dangerous because it corrupts
> whatever is on the address calculated by SRCU. Thus, CC'ing the stable
> list.
> 
> [1]: commit 9d7de2aa8b41 ("x86/percpu/64: Use relative percpu offsets")
> [2]: commit 9286675a2aed ("HID: bpf: add HID-BPF hooks for
> hid_hw_output_report")
> 
> Closes: https://lore.kernel.org/all/20250506145548.GGaBoi9Jzp3aeJizTR@fat_crate.local/
> Fixes: 8bd0488b5ea5 ("HID: bpf: add HID-BPF hooks for hid_hw_raw_requests")
> Cc: stable@...r.kernel.org
> Signed-off-by: Rong Zhang <i@...g.moe>

Yes, this patch fixes the BUG and subsequent lock-ups in my scenario
(suspend with a Bluetooth keyboard). Thank you!

Tested-by: Petr Tesarik <petr@...arici.cz>

Petr T

> ---
>  drivers/hid/bpf/hid_bpf_dispatch.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c
> b/drivers/hid/bpf/hid_bpf_dispatch.c index 2e96ec6a3073..9a06f9b0e4ef
> 100644 --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -38,6 +38,9 @@ dispatch_hid_bpf_device_event(struct hid_device
> *hdev, enum hid_report_type type struct hid_bpf_ops *e;
>  	int ret;
>  
> +	if (unlikely(hdev->bpf.destroyed))
> +		return ERR_PTR(-ENODEV);
> +
>  	if (type >= HID_REPORT_TYPES)
>  		return ERR_PTR(-EINVAL);
>  
> @@ -93,6 +96,9 @@ int dispatch_hid_bpf_raw_requests(struct hid_device
> *hdev, struct hid_bpf_ops *e;
>  	int ret, idx;
>  
> +	if (unlikely(hdev->bpf.destroyed))
> +		return -ENODEV;
> +
>  	if (rtype >= HID_REPORT_TYPES)
>  		return -EINVAL;
>  
> @@ -130,6 +136,9 @@ int dispatch_hid_bpf_output_report(struct
> hid_device *hdev, struct hid_bpf_ops *e;
>  	int ret, idx;
>  
> +	if (unlikely(hdev->bpf.destroyed))
> +		return -ENODEV;
> +
>  	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))
> {
> 
> base-commit: 82f2b0b97b36ee3fcddf0f0780a9a0825d52fec3