Re: [PATCH v7 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 access to sibling `hid_device` pointers (`shared->touch` and `shared->pen`) in `wacom_mode_change_work()` causes a TOCTOU race leading to Use-After-Free if a sibling device is disconnected concurrently.
- [High] Race condition between `kref_put()` in `wacom_remove_shared_data()` and `kref_get()` in `wacom_get_hdev_data()` leads to Use-After-Free of the shared data structure.
--

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

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

This commit introduces RCU protection for the touch_input pointer to resolve
a TOCTOU race condition in wacom_intuos_pad(). It also serializes probe
assignments and removal in the shared data struct using wacom_udev_list_lock.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> --- 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 isn't a bug introduced by this patch, but it looks like shared->pen and
shared->touch can still be accessed locklessly in wacom_mode_change_work().

If the sibling pen device is dynamically unbound via sysfs or unplugged,
this path clears the pointer under the lock, frees the wacom structure,
and drops the hid_device reference.

Could a preempted wacom_mode_change_work() worker thread read the valid
shared->pen pointer before it is cleared, resume after the unbind completes,
and then dereference the freed pointer?

    if (shared->pen) {
        wacom1 = hid_get_drvdata(shared->pen);
        wacom_release_resources(wacom1);
        hid_hw_stop(wacom1->hdev);
    }

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

[Severity: High]
This is a pre-existing issue, but I have a question about this kref_put()
call happening outside of the wacom_udev_list_lock.

If the refcount drops to 0, wacom_release_shared_data() is queued and will
block on acquiring wacom_udev_list_lock to remove the data object from
wacom_udev_list.

Concurrently, if wacom_add_shared_data() is probing on another CPU, it holds
the mutex and calls wacom_get_hdev_data(). That function iterates the list
and calls kref_get() unconditionally on the matching item:

    list_for_each_entry(data, &wacom_udev_list, list) {
        if (hid_compare_device_paths(hdev, data->dev, '/')) {
            kref_get(&data->kref);
            return data;
        }
    }

Does this create a race where wacom_get_hdev_data() can acquire a reference
to a 0-refcount object just before wacom_release_shared_data() removes it
and calls kfree(data), leading to a use-after-free in the concurrent probe?
Should this perhaps use kref_put_mutex() to ensure atomicity?

[ ... ]

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