Re: [PATCH v4 1/3] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free in `wacom_mode_change_work()` due to unprotected lockless access of sibling device pointers.
--

commit 2e75e17f7579b493b3120751d2b8cfa23bc2c814
Author: Lee Jones <lee@kernel.org>

HID: wacom: Fix Use-After-Free in wacom_intuos_pad

This commit resolves a Time-of-Check to Time-of-Use race condition by
introducing RCU protection for the touch_input pointer. It also wraps
shared value assignments in the udev list lock to prevent concurrent
desynchronization, and advertises the SW_MUTE_DEVICE capability before
registration.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..86895f13dbaea 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -877,10 +877,16 @@ static void wacom_remove_shared_data(void *res)
>  		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
>  				    shared);
>  
> -		if (wacom_wac->shared->touch == wacom->hdev)
> -			wacom_wac->shared->touch = NULL;
> -		else if (wacom_wac->shared->pen == wacom->hdev)
> -			wacom_wac->shared->pen = NULL;
> +		scoped_guard(mutex, &wacom_udev_list_lock) {
> +			if (wacom_wac->shared->touch == wacom->hdev) {
> +				wacom_wac->shared->touch = NULL;
> +				rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
> +			} else if (wacom_wac->shared->pen == wacom->hdev) {
> +				wacom_wac->shared->pen = NULL;
> +			}
> +		}
> +
> +		synchronize_rcu();

[Severity: High]
This is a pre-existing issue, but I noticed a potential Use-After-Free in
wacom_mode_change_work() due to unprotected lockless access of sibling device
pointers.

While synchronize_rcu() is called here to ensure active RCU readers have
finished, wacom_mode_change_work() accesses sibling pointers locklessly
without an rcu_read_lock():

wacom_mode_change_work() {
        ...
        struct hid_device *pen = rcu_access_pointer(shared->pen);
        struct hid_device *touch = rcu_access_pointer(shared->touch);
        ...
        if (pen) {
                wacom1 = hid_get_drvdata(pen);
        ...
}

Because there is no rcu_read_lock() wrapping the access in
wacom_mode_change_work(), this synchronize_rcu() call will return without
waiting for the preempted work to finish.

If a sibling device (like the pen device) is concurrently disconnected,
wacom_remove() cancels the pen device's mode change work but leaves the
touch device's work running.

When wacom_remove_shared_data() executes for the pen, it nullifies
shared->pen and calls synchronize_rcu(). Since it won't wait for the
unprotected touch device's work, the pen device memory could be freed by
the HID core before that work resumes.

When the work eventually resumes and calls hid_get_drvdata(pen), won't it
dereference the freed pen pointer and cause a Use-After-Free?

I see from the patch 2 commit message that this lockless access is
intentionally left as-is because wacom_mode_change_work() sleeps and cannot
use RCU read locks. Could an alternative approach be used here to prevent
this use-after-free?

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