Re: [PATCH v3 4/8] HID: asus: cleanup keyboard listener on failure: avoid use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The newly added `err_unregister_backlight` error path frees `drvdata->kbd_backlight` without canceling its associated workqueue, leading to a Use-After-Free.
- [High] The `asus_probe` error path fails to unregister the dynamically registered `xgm_led` class device, leaving a dangling pointer in the LED subsystem.

Pre-existing issues:
- [High] The `asus_probe` error paths fail to cancel `fn_lock_sync_work` and `wmi_work`, creating a potential Use-After-Free if the works are scheduled during probe.
--

commit 6e04c2967d627dea02497ed7471439a5d35c56bc
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: cleanup keyboard listener on failure: avoid use-after-free

This commit fixes a potential use-after-free by unregistering the keyboard
listener on probe failure. It explicitly adds an error path to unregister
the listener and free the memory when a subsequent initialization step fails.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 95e7dc6a447d8..e3a03f7cf77d0 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1420,11 +1420,17 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		if (drvdata->tp) {
>  			ret = asus_start_multitouch(hdev);
>  			if (ret)
> -				goto err_stop_hw;
> +				goto err_unregister_backlight;
>  		}
>  	}
>  
>  	return 0;
> +err_unregister_backlight:
> +	if (drvdata->kbd_backlight) {
> +		asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
> +		devm_kfree(&hdev->dev, drvdata->kbd_backlight);

[Severity: High]
Does this code leave pending work items referencing freed memory?

If concurrent events have queued `drvdata->kbd_backlight->work` (e.g.,
via `asus_kbd_backlight_set()` or `asus_hid_event()`), freeing
`kbd_backlight` without synchronizing via `cancel_work_sync()` might
cause a use-after-free when the workqueue executes.

> +		drvdata->kbd_backlight = NULL;
> +	}
>  err_stop_hw:
>  	hid_hw_stop(hdev);
>  	return ret;

[Severity: High]
Could failing to unregister `xgm_led` leave a dangling pointer in the LED
subsystem?

If `asus_xgm_init()` succeeds, it registers `drvdata->xgm_led->cdev` using
`led_classdev_register()`, which is not devres-managed. If `asus_probe()`
subsequently fails and jumps here, the code exits without calling
`led_classdev_unregister()`. Since `drvdata->xgm_led` is devres-managed, it
will be freed, but the LED subsystem will retain a pointer to it,
potentially causing a use-after-free.

[Severity: High]
This is a pre-existing issue, but does this error path also need to cancel
`fn_lock_sync_work` and `wmi_work`?

Once the hardware is started via `hid_hw_start()`, incoming input events
can schedule `drvdata->fn_lock_sync_work` or `drvdata->wmi_work`. If
`asus_probe()` fails and exits here, devres will automatically free
`drvdata`. Since `cancel_work_sync()` is omitted from this error path, any
queued work could execute asynchronously after `drvdata` is freed, leading
to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613153029.2559774-1-denis.benato@linux.dev?part=4