Re: [PATCH 3/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_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` in `wacom_mode_change_work()` leads to Use-After-Free if a sibling device is disconnected.
- [High] Race condition between `kref_put()` and `kref_get()` for `wacom_hdev_data` leads to Use-After-Free.
--

--- Patch [3]: [PATCH 3/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad ---
commit ba24ac0ee968766a5b0db4400d76393921713f71
Author: Lee Jones <lee@kernel.org>

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

This commit introduces RCU protection for pen and touch pointers in the
wacom_shared struct to resolve a use-after-free issue. It wraps lockless
readers with RCU read locks and updates writers to use RCU assignment and
dereferencing functions.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index c6b37931d02d1..5578de80f4c80 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -876,11 +876,18 @@ static void wacom_remove_shared_data(void *res)
>  				    shared);
>  
>  		scoped_guard(mutex, &wacom_udev_list_lock) {
> -			if (wacom_wac->shared->touch == wacom->hdev) {
> -				wacom_wac->shared->touch = NULL;
> +			struct hid_device *touch =
> +				rcu_dereference_protected(wacom_wac->shared->touch,
> +							  lockdep_is_held(&wacom_udev_list_lock));
> +			struct hid_device *pen =
> +				rcu_dereference_protected(wacom_wac->shared->pen,
> +							  lockdep_is_held(&wacom_udev_list_lock));
> +
> +			if (touch == wacom->hdev) {
> +				rcu_assign_pointer(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;
> +			} else if (pen == wacom->hdev) {
> +				rcu_assign_pointer(wacom_wac->shared->pen, NULL);
>  			}
>  		}
>  

[Severity: High]
This is a pre-existing issue, but is there a race condition between
kref_put() and kref_get() for wacom_hdev_data that could lead to a
use-after-free?

In wacom_remove_shared_data(), kref_put() is called without holding
wacom_udev_list_lock:

	kref_put(&data->kref, wacom_release_shared_data);

If the refcount drops to 0, it calls wacom_release_shared_data(), which
acquires the lock to remove the element.

In the window before the lock is acquired, could another thread in
wacom_add_shared_data() iterate over wacom_udev_list, find the still-linked
data, and call kref_get(&data->kref)?

This would increment a 0-refcount object, and subsequently the first thread
frees it, leaving the second thread with a dangling pointer.

[ ... ]

> @@ -2793,16 +2804,19 @@ static void wacom_mode_change_work(struct work_struct *work)
>  	bool is_direct = wacom->wacom_wac.is_direct_mode;
>  	int error = 0;
>  
> -	if (shared->pen) {
> -		wacom1 = hid_get_drvdata(shared->pen);
> +	struct hid_device *pen = rcu_access_pointer(shared->pen);
> +	struct hid_device *touch = rcu_access_pointer(shared->touch);
> +
> +	if (pen) {
> +		wacom1 = hid_get_drvdata(pen);

[Severity: High]
This is a pre-existing issue, but does lockless access to shared->pen and
shared->touch in wacom_mode_change_work() lead to a use-after-free if a
sibling device is disconnected?

Pointers are fetched locklessly via rcu_access_pointer() and subsequently
dereferenced without any synchronization. If a sibling device is disconnected,
wacom_remove_shared_data() nullifies the pointer and calls synchronize_rcu().

Because wacom_mode_change_work() runs asynchronously and reads the pointer
without holding rcu_read_lock() or wacom_udev_list_lock, synchronize_rcu()
does not wait for it.

Can this cause the sibling device's memory to be freed and subsequently
dereferenced by the worker thread (e.g., via hid_get_drvdata(pen))?

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