Re: [PATCH v6 2/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to sibling device pointers in wacom_mode_change_work() leads to concurrent double-free and Use-After-Free.
- [High] wacom_remove_shared_data() calls kref_put() outside of the list lock, creating a race condition that leads to refcount corruption and Use-After-Free.
- [High] The patch protects touch_input with RCU but fails to protect shared->pen and shared->touch, leaving a Use-After-Free vulnerability in IRQ paths.
--

--- Patch [2]: [PATCH v6 2/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad ---
commit db13f6a5a689d086da00c273947930e39aedbb61
Author: Lee Jones <lee@kernel.org>

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

This patch attempts to fix a TOCTOU Use-After-Free race condition by
introducing RCU protection for the touch_input pointer in the wacom_shared
struct, wrapping readers in RCU guards, updating writers, and calling
synchronize_rcu().

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 6c2a3ddfe366a..c6b37931d02d1 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -875,10 +875,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;
> +			}
> +		}

[Severity: High]
This is a pre-existing issue, but does shared->pen (and shared->touch) need
the same RCU protection that is being added here for touch_input?

For instance, in wacom_bamboo_pad_pen_event(), shared->pen is accessed
locklessly in an interrupt handler:

drivers/hid/wacom_wac.c:wacom_bamboo_pad_pen_event() {
    ...
    hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data,
                     WACOM_PKGLEN_PENABLED, 1);
    ...
}

If the Pen sibling device is disconnected concurrently,
wacom_remove_shared_data() clears shared->pen here. Could a preempted or
concurrent IRQ read the non-NULL shared->pen pointer before it is cleared,
and pass the dangling pointer to hid_input_report() once the Pen device is
unbound and freed?

Additionally, in wacom_mode_change_work(), there is lockless access to
these sibling device pointers:

drivers/hid/wacom_sys.c:wacom_mode_change_work() {
    ...
    wacom1 = hid_get_drvdata(shared->pen);
    wacom_release_resources(wacom1);
    hid_hw_stop(wacom1->hdev);
    ...
}

Could the asynchronous worker on a Touch device read shared->pen concurrently
with the Pen device being unbound, leading to a concurrent double-free or
Use-After-Free of the device memory?

> +
> +		synchronize_rcu();
>  
>  		kref_put(&data->kref, wacom_release_shared_data);

[Severity: High]
This isn't a bug introduced by this patch, but since kref_put() is called
here outside of the wacom_udev_list_lock, can this create a race condition
that leads to refcount corruption?

If the refcount drops to 0, it calls the release function which waits for
the lock to remove the object from wacom_udev_list. Concurrently, couldn't a
probe on another device acquire the lock and iterate the list:

drivers/hid/wacom_sys.c:wacom_get_hdev_data() {
    ...
    kref_get(&data->kref);
    return data;
    ...
}

Could this result in incrementing a 0-refcount object, causing a
Use-After-Free when the original thread proceeds to unlink and free the
structure?

>  		wacom_wac->shared = NULL;
>  	}
>  }

[ ... ]

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