On Mon, 12 May 2025 23:24:19 +0800
Rong Zhang <i@xxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Rong Zhang <i@xxxxxxxx>
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@xxxxxxxxxxx>
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
