Re: [PATCH v7 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] wacom_mode_change_work() accesses shared->pen and shared->touch locklessly outside of an RCU read-side critical section, leading to a Use-After-Free if a sibling device is removed.
- [High] wacom_remove_shared_data() calls kref_put() without holding wacom_udev_list_lock, creating a race condition where a concurrent probe can resurrect the object while it is being freed.
--

--- Patch [3]: [PATCH v7 3/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad ---
commit 2aec7c2beec55f0a42e84d2a747129503b809e66
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_bamboo_pad driver. It adds RCU annotations and wraps lockless readers 
in rcu_read_lock() to prevent Use-After-Free bugs when sibling devices 
are removed.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index cf9c12f3cff53..05d27fcd311f8 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 isn't a bug introduced by this patch, but does wacom_remove_shared_data()
have a race condition when calling kref_put()?

If kref_put(&data->kref, wacom_release_shared_data) is executed without holding
wacom_udev_list_lock, it drops the refcount to 0 and jumps to the release 
callback wacom_release_shared_data().

Before the release callback acquires wacom_udev_list_lock, a concurrent device
probe executing wacom_add_shared_data() could iterate over wacom_udev_list and
call kref_get() on the zero-refcount object.

Could a concurrent probe resurrect the object while it is being freed, leading
to a Use-After-Free?

[ ... ]

> @@ -2810,16 +2821,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 wacom_mode_change_work() have a
Use-After-Free risk because it accesses the pointers without RCU read-side
protection?

Since this workqueue does not hold rcu_read_lock(), a concurrent unbind
operation calling synchronize_rcu() can return immediately.

If a sibling device is unbound concurrently, could the pointer be freed while
this function continues to dereference it via hid_get_drvdata() and calls
blocking functions like hid_hw_stop()?

>  		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);
>  		wacom1->wacom_wac.has_mode_change = true;

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