Re: [PATCH v3 4/4] HID: wacom: Fix teardown order in wacom_mode_change_work

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing synchronization between sibling device removal and `wacom_mode_change_work` leads to Use-After-Free and double `hid_hw_stop()`.
- [Critical] Missing synchronization allows multiple `wacom_mode_change_work` instances to execute concurrently on both siblings, leading to double teardown and state corruption.
- [Critical] Error paths in `wacom_parse_and_register()` after `hid_hw_start()` fail to call `hid_hw_stop()`, leaving hardware IRQs active after releasing resources.
- [High] If `wacom_parse_and_register()` fails for the first sibling device, the second device is permanently left in a stopped and uninitialized state.
- [High] Pending background workqueues and timers are not cancelled before releasing device resources in `wacom_mode_change_work`, violating subsystem teardown guidelines and causing UAF.
- [High] `wacom_remove()` cancels `mode_change_work` after stopping the hardware, creating a race that leaves the device started and permanently leaks HID interfaces.
--

commit 0812538ee6f90ea0ab3dbb1250c3e8460816a993
Author: Lee Jones <lee@kernel.org>

HID: wacom: Fix teardown order in wacom_mode_change_work

wacom_mode_change_work() called wacom_release_resources() before
hid_hw_stop(), which could lead to a crash if hardware IRQs fire and
dereference the nullified shared pointer. This commit fixes the teardown
order to call hid_hw_stop() first.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 1b019e3331b45..378dddbf7ec5b 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2835,16 +2835,16 @@ static void wacom_mode_change_work(struct work_struct *work)
>  
>  	if (pen) {

[Severity: Critical]
This is a pre-existing issue, but is it possible for the shared device
pointers to be dereferenced after they are freed?

In wacom_mode_change_work(), the sibling device pointers are fetched:

    struct hid_device *pen = rcu_access_pointer(shared->pen);
    ...

These pointers are accessed without holding an rcu_read_lock or acquiring
a device reference. If a sibling device is concurrently unbound,
wacom_remove() clears the shared pointer and calls synchronize_rcu(), but
it would not wait for this worker. Could this result in a use-after-free
when hid_hw_stop() is called on the stale pointer?

[Severity: Critical]
This isn't a bug introduced by this patch, but does this function have
sufficient synchronization if mode change events arrive concurrently on
both pen and touch interfaces?

Since the code unconditionally operates on both sibling devices without
locking:

    wacom1 = hid_get_drvdata(pen);
    hid_hw_stop(wacom1->hdev);
    wacom_release_resources(wacom1);

If both devices independently schedule their own mode_change_work, could
both workers run simultaneously and race to call wacom_release_resources()
on the exact same pointers, potentially causing a double-free in the
devres core?

>  		wacom1 = hid_get_drvdata(pen);
> -		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);
> +		wacom_release_resources(wacom1);

[Severity: High]
This is a pre-existing issue, but are we certain all background tasks
are cancelled before releasing resources here?

Unlike wacom_remove(), wacom_mode_change_work() does not cancel pending
tasks like init_work before calling wacom_release_resources(). Since
wacom_release_resources() destroys devres allocations (such as led groups),
could a delayed wacom_init_work() execute concurrently and access these
freed structures, resulting in a use-after-free?

>  		wacom1->wacom_wac.has_mode_change = true;
>  		wacom1->wacom_wac.is_direct_mode = is_direct;
>  	}
>  
>  	if (touch) {
>  		wacom2 = hid_get_drvdata(touch);
> -		wacom_release_resources(wacom2);
>  		hid_hw_stop(wacom2->hdev);
> +		wacom_release_resources(wacom2);
>  		wacom2->wacom_wac.has_mode_change = true;
>  		wacom2->wacom_wac.is_direct_mode = is_direct;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but what happens to the second
device if re-registration fails for the first one?

Looking at the rest of wacom_mode_change_work():

    if (wacom1) {
        error = wacom_parse_and_register(wacom1, false);
        if (error)
            return;
    }
    if (wacom2) {
        ...
    }

If wacom_parse_and_register() fails for wacom1, the early return bypasses
the initialization for wacom2. Does this permanently leave the touch
interface hardware stopped and its resources released without recovery?

[Severity: Critical]
This is a pre-existing issue, but is there a missing hardware teardown
in the error handling path of wacom_parse_and_register()?

When an error occurs after hid_hw_start() succeeds:

drivers/hid/wacom_sys.c:wacom_parse_and_register() {
    ...
fail_quirks:
    hid_hw_stop(hdev);
fail:
    wacom_release_resources(wacom);
    return error;
}

The goto fail jumps past hid_hw_stop(hdev). Does this leave the hid
hardware running while its resources are freed, allowing subsequent
hardware irqs to trigger wacom_raw_event() and cause a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does wacom_remove()
cancel the mode_change_work too late?

In wacom_remove(), the cancellation happens after stopping the hardware:

drivers/hid/wacom_sys.c:wacom_remove() {
    hid_hw_stop(hdev);
    cancel_delayed_work_sync(&wacom->init_work);
    ...
    cancel_work_sync(&wacom->mode_change_work);
}

If mode_change_work is pending and runs after hid_hw_stop(), it will
call wacom_parse_and_register(), which invokes hid_hw_start(hdev).
Once cancel_work_sync() returns, wacom_remove() completes without
re-stopping the hardware. Could this permanently leak the active hid
interfaces?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121353.3743782-1-lee@kernel.org?part=4