Re: [PATCH 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless accesses to `shared->pen` and `shared->touch` cause a TOCTOU Use-After-Free race condition during concurrent sibling device removal.
- [High] Improper `kref_put()` usage during shared data removal causes a Use-After-Free of `wacom_hdev_data` due to a race with list traversal.
--

--- Patch [2]: [PATCH 2/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad ---
commit b17a0dd077bc2659cd08a43647ccf55bb0fc7cb0
Author: Lee Jones <lee@kernel.org>
Subject: 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
concurrent probe assignments in wacom_udev_list_lock.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 92b73ed0028f1..cf9c12f3cff53 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 was not introduced by this patch. Should
shared->pen (and shared->touch) also be annotated and cleared with
rcu_assign_pointer()?

There appear to be lockless readers of shared->pen and shared->touch in
the input event handlers, such as wacom_bamboo_pad_pen_event() calling
hid_input_report() or wacom_mode_change_work() dereferencing it via
hid_get_drvdata(). If a sibling device is unbound concurrently, those
readers might access a freed hid_device.

Since they don't hold rcu read locks, the synchronize_rcu() added below
won't wait for them unless they are also updated.

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

[Severity: High]
This is a pre-existing issue, but can this kref_put() cause a use-after-free
due to a race with list traversal?

If this drops the reference count to 0, wacom_release_shared_data() is
called. Concurrently, wacom_add_shared_data() might acquire the
wacom_udev_list_lock and iterate the list.

If it finds this data object before it is removed from the list, it will
call kref_get() on a 0-refcount object via wacom_get_hdev_data() and return
it.

Once the lock is released, the release function could proceed to kfree()
the data object, and the probe thread would access the freed struct.
Should this use kref_put_mutex() with wacom_udev_list_lock to safely
synchronize with the list iteration?

>  		wacom_wac->shared = NULL;

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