Re: [PATCH v10 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] Lockless access to `shared->pen` and `shared->touch` leaves sibling `hid_device` pointers vulnerable to Use-After-Free during concurrent hot-unplug.
- [High] Sysfs callbacks access `wacom->led.groups` without NULL checks, racing with `wacom_remove()` cleanup.
--

commit f9df2fb1ac73b67929633c2e25c6e475a7abd240
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. If the Touch sibling device
is disconnected, wacom_remove_shared_data() clears 'touch_input'
outside any lock, creating a Time-of-Check to Time-of-Use (TOCTOU)
race condition where a preempted reader in interrupt context
dereferences the freed pointer, leading to a Use-After-Free.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 4eed2c189017d..10f7de515ea28 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;

[Severity: High]
This is a pre-existing issue, but since this patch introduces RCU protection
for touch_input, is it possible that lockless access to shared->pen and
shared->touch in wacom_mode_change_work() is still vulnerable to a
Use-After-Free during concurrent hot-unplug?

When a sibling interface is unbound, devres automatically frees its driver
data. If a WACOM_HID_WD_MODE_CHANGE event concurrently schedules
wacom_mode_change_work(), it accesses these pointers locklessly:

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 (shared->touch) {
		wacom2 = hid_get_drvdata(shared->touch);
		wacom_release_resources(wacom2);
		hid_hw_stop(wacom2->hdev);
    ...
}

While Patch 5/5 in this series attempts to fix this by replacing the raw
accesses with rcu_access_pointer(), it appears to omit wrapping the worker
in an RCU read-side critical section and doesn't use rcu_dereference().

Because of this, would the synchronize_rcu() call in the unbind path fail to
wait for this worker, leaving the Use-After-Free vulnerability unresolved
at the end of the series?

[ ... ]

[Severity: High]
This is a pre-existing issue, but could wacom_show_remote_mode() race with
wacom_remove() and cause a NULL pointer dereference?

During unbind, wacom_remove() explicitly nullifies the groups pointer before
devres removes the sysfs attributes:

drivers/hid/wacom_sys.c:wacom_remove() {
    ...
	/* make sure we don't trigger the LEDs */
	wacom_led_groups_release(wacom);

	if (wacom->wacom_wac.features.type != REMOTE)
		wacom_release_resources(wacom);
}

If an unprivileged local user continuously reads the world-readable sysfs
files during a hot-unplug, the sysfs callback dereferences wacom->led.groups
without a NULL check:

drivers/hid/wacom_sys.c:wacom_show_remote_mode() {
    ...
	mode = wacom->led.groups[index].select;
	return sprintf(buf, "%d\n", mode < 3 ? mode : -1);
}

Could this crash the kernel if the sysfs read callback executes after
wacom_led_groups_release() clears the pointer, but before devres cleanup
finishes?

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