Re: [PATCH v9 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 asynchronous work queues and timers in the probe error path after hardware start.
- [High] Lockless access to `shared->touch` and `shared->pen` in `wacom_mode_change_work` can lead to Use-After-Free during concurrent device removal.
--

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

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

    This patch addresses a Use-After-Free race condition in wacom_intuos_pad()
    by adding RCU protection to the touch_input pointer. It also wraps device
    assignments in wacom_udev_list_lock to serialize concurrent probe
    assignments.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 026a6be467d3d..1af0d518260d6 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();
>  
>  		kref_put(&data->kref, wacom_release_shared_data);
>  		wacom_wac->shared = NULL;

[ ... ]

While reviewing this code, I noticed a couple of pre-existing issues in the
surrounding functions.

drivers/hid/wacom_sys.c:wacom_parse_and_register() {
    ...
	error = hid_hw_start(hdev, connect_mask);
	if (error) {
		hid_err(hdev, "hw start failed\n");
		goto fail;
	}

	error = wacom_register_inputs(wacom);
	if (error)
		goto fail_hw_stop;
    ...
fail_hw_stop:
	hid_hw_stop(hdev);
fail:
	wacom_release_resources(wacom);
	return error;
}

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does this error path omit the cancellation of asynchronous
resources?

Once hid_hw_start() is called, the device hardware can generate interrupts
and schedule background work such as wireless_work, battery_work, or the
idleprox_timer. If a subsequent initialization step fails, the probe error
path calls hid_hw_stop() and wacom_release_resources(), and then returns.
This causes devres to free the wacom structure. Because the scheduled work
is not explicitly canceled using cancel_work_sync() or timer_delete_sync(),
the work could execute later and access the freed structure, leading to a
Use-After-Free.

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);
		wacom1->wacom_wac.has_mode_change = true;
    ...
}

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but could this lockless access to sibling devices lead to a
Use-After-Free?

The wacom_mode_change_work function locklessly accesses sibling device
pointers like shared->pen and shared->touch. If the sibling device is
concurrently hot-unplugged and unbound, the synchronize_rcu() call in the
unbind path does not block this work from using a cached pointer, since
there is no RCU read-side critical section (such as guard(rcu)()). This
allows hid_get_drvdata() to dereference a freed device structure, resulting
in a Use-After-Free.

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