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] Pre-existing use-after-free race between kref_put() in wacom_remove_shared_data() and list traversal in wacom_get_hdev_data().
- [High] Lockless reads of `shared->pen` and `shared->touch` lead to TOCTOU Use-After-Free.
--

--- Patch [2]: [PATCH 2/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad ---
commit 7367a08b48f1d1fc2e7dc8823f95547c3c03eb51
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.

Resolve this by introducing RCU protection for the touch_input
pointer.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 6c2a3ddfe366a..c6b37931d02d1 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 isn't a bug introduced by this patch, but since we are introducing
RCU protection for touch_input, do shared->pen and shared->touch need
similar RCU annotations and read guards?

wacom_mode_change_work() reads both pointers locklessly in preemptible
workqueue context:

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

Similarly, wacom_bamboo_pad_pen_event() dereferences shared->pen without
protection:

drivers/hid/wacom_wac.c:wacom_bamboo_pad_pen_event() {
    ...
    hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data, ...);
    ...
}

Because these readers run without rcu_read_lock() or guard(rcu), the new
synchronize_rcu() call won't wait for them, potentially leaving them to
dereference freed pointers when the sibling device unbinds. Should pen and
touch also be converted to RCU pointers?

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

[Severity: High]
This is a pre-existing issue, but does this lockless kref_put() create a
race condition with concurrent device probes?

If a concurrent unbind and probe occurs, kref_put() can drop the refcount
to 0 and synchronously call wacom_release_shared_data(). Before the
release function can acquire the wacom_udev_list_lock to remove the
object, a concurrent wacom_add_shared_data() might call
wacom_get_hdev_data().

wacom_get_hdev_data() iterates the list under the lock, finds the still-
linked object, and blindly calls kref_get(&data->kref) on the 0-refcount
object:

drivers/hid/wacom_sys.c:wacom_get_hdev_data() {
    ...
    list_for_each_entry(data, &wacom_udev_list, list) {
        if (strncmp(data->phys, hdev->phys, sizeof(data->phys) - 2) == 0) {
            kref_get(&data->kref);
            return data;
        }
    ...
}

When the lock is dropped, the release function proceeds to kfree() the
object, leaving the prober with a freed pointer. Should this be using
kref_put_mutex() or kref_get_unless_zero() to safely handle the list
removal?

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