Re: [PATCH 3/5] 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] Missing cancellation of delayed work on probe error path leads to Use-After-Free.
- [High] Lockless accesses to sibling pointers lead to TOCTOU Use-After-Free if a sibling device is disconnected concurrently.
--

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

    HID: wacom: Fix Use-After-Free in wacom_intuos_pad
    
    wacom_intuos_pad() accesses wacom->shared->touch_input locklessly
    inside the interrupt handler context, leading to a Use-After-Free if
    the sibling device is disconnected. This patch resolves the race by
    introducing RCU protection for the touch_input pointer.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2738d4f515e6a..b88726cbbb4ec 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;
> +			}
> +		}
> +
> +		synchronize_rcu();

[Severity: High]
This isn't a bug introduced by this patch, but does wacom_mode_change_work()
still have a TOCTOU Use-After-Free race with this function?

wacom_mode_change_work() accesses the shared->pen and shared->touch pointers
locklessly, dereferences them via hid_get_drvdata(), and then calls blocking
operations:

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

If a sibling device is concurrently disconnected, its wacom_remove_shared_data()
executes. The synchronize_rcu() added here does not protect process-context
readers in wacom_mode_change_work() that do not hold an RCU read lock.

The disconnected sibling's devres cleanup runs, freeing the sibling's memory,
while the concurrent wacom_mode_change_work() from the still-connected sibling
dereferences the freed pointer.

Can we safely access these pointers in wacom_mode_change_work() without holding
wacom_udev_list_lock or using another mechanism to prevent the sibling from
being freed?

[ ... ]

> @@ -2343,9 +2349,15 @@ static void wacom_release_resources(struct wacom *wacom)
>  
>  static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
>  {
> +	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> +
> +	guard(mutex)(&wacom_udev_list_lock);
> +
>  	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
> -		wacom_wac->shared->type = wacom_wac->features.type;
> -		wacom_wac->shared->touch_input = wacom_wac->touch_input;
> +		if (wacom_wac->shared->touch == wacom->hdev) {
> +			wacom_wac->shared->type = wacom_wac->features.type;
> +			rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but does wacom_parse_and_register() leak the
wacom->init_work on its error path, potentially leading to a Use-After-Free?

During probe, wacom_query_tablet_data() schedules wacom->init_work with
a 1-second delay. If a subsequent initialization step fails (such as
hid_hw_open() returning an error), the code jumps to the error path, which calls
wacom_release_resources() but fails to cancel the delayed work:

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

The probe function will propagate the error and devres will automatically free
the wacom structure. When the timer expires, the delayed work will execute and
dereference the freed wacom structure.

Should we add a cancel_delayed_work_sync(&wacom->init_work) in this error path?

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