Re: [PATCH v3 6/8] HID: asus: prevent a late KEY_FN_ESC to trigger a use-after-free

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unvalidated `size` from an untrusted HID event directly overwrites `drvdata->wmi_size`, leading to a stack buffer overflow in `asus_wmi_work()`.
- [High] Missing `led_classdev_unregister` for `xgm_led` on `asus_probe` error paths leading to a Use-After-Free.

Pre-existing issues:
- [High] Missing `cancel_work_sync` on the `asus_probe` error path can lead to a use-after-free of the `kbd_backlight->work` workqueue structure.
--

commit 621a1df344c100c1c65a96b5fd1d410d256fafef
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: prevent a late KEY_FN_ESC to trigger a use-after-free

A late KEY_FN_ESC event can trigger asus_event() and unconditionally call
schedule_work() after the work was supposedly canceled.

Once the driver is removed and drvdata is freed by devres, the workqueue
could execute asus_sync_fn_lock() and dereference the freed drvdata.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index a3437789b2341..c1a247348c3d9 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

[Severity: Critical]
Can this lead to a stack buffer overflow?

Unvalidated size from an untrusted HID event directly overwrites
drvdata->wmi_size in the commit 'HID: asus: avoid sleeping calls in atomic
context':

drivers/hid/hid-asus.c:asus_raw_event() {
    ...
    if (data[1] == ASUS_FAN_CTRL_KEY_CODE) {
        ...
        drvdata->wmi_size = size;
    ...
}

Because size comes from the transport layer, a malicious USB device can
supply an event size up to HID_MAX_BUFFER_SIZE (4096 bytes).

Later, the asynchronously scheduled asus_wmi_work() uses this oversized
wmi_size:

drivers/hid/hid-asus.c:asus_wmi_work() {
    ...
    u8 report_data[FEATURE_KBD_REPORT_SIZE];
    ...
    memcpy(report_data, drvdata->wmi_data, drvdata->wmi_size);
    ...
}

Will this overflow the 64-byte stack buffer report_data?

[Severity: High]
This is a pre-existing issue, but does this error path need a call to
cancel_work_sync(&drvdata->kbd_backlight->work)?

drivers/hid/hid-asus.c:asus_probe() {
    ...
err_unregister_backlight:
    if (drvdata->kbd_backlight) {
        asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
        devm_kfree(&hdev->dev, drvdata->kbd_backlight);
        drvdata->kbd_backlight = NULL;
    }
    ...
}

If a subsequent initialization step fails, the error path unregisters the
listener and frees the backlight structure.

If the work was queued prior to the failure, wouldn't the workqueue core
execute the pending work on the newly freed memory?

[Severity: High]
Does this code leave a dangling pointer in the LED subsystem list?

drivers/hid/hid-asus.c:asus_probe() {
    ...
err_stop_hw:
    hid_hw_stop(hdev);
    return ret;
}

In the commit 'HID: asus: add support for xgm led', asus_xgm_init()
allocates drvdata->xgm_led using devm_kzalloc() and registers it via
led_classdev_register().

If a subsequent initialization step fails, the error handling paths branch
to err_unregister_backlight and err_stop_hw without calling
led_classdev_unregister() for the XGM LED.

When asus_probe() returns, devres will automatically free drvdata->xgm_led,
but won't unregister the device, which could leave a dangling pointer.

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