Re: [PATCH v3 2/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `wacom_mode_change_work()` misuses `rcu_access_pointer()` to fetch RCU-protected pointers without holding `rcu_read_lock()`, leading to a Use-After-Free.
--

commit 4eb3f98aa9f594548c94852bcf57eb40ae946cd8
Author: Lee Jones <lee@kernel.org>
Subject: HID: wacom: Fix Use-After-Free in wacom_bamboo_pad

This commit resolves a Use-After-Free vulnerability in
wacom_bamboo_pad_pen_event() by introducing RCU protection for the pen and
touch pointers within the wacom_shared structure. It annotates the pointers
with __rcu, wraps lockless read access with rcu_read_lock() and
rcu_dereference(), and updates writers using the appropriate RCU assignment
primitives. It also updates pointer access in wacom_mode_change_work() to use
rcu_access_pointer() to avoid warnings.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 7ba5898265481..9b352027aa980 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2797,16 +2808,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);
>  		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);

[Severity: High]
This isn't a bug introduced by this patch, but does wacom_mode_change_work()
misuse rcu_access_pointer() here by fetching RCU-protected pointers without
holding rcu_read_lock(), which still leaves an exploitable Use-After-Free?

If the sibling device is unbound concurrently, its wacom_remove() triggers
wacom_remove_shared_data(), which nullifies the shared pointer and calls
synchronize_rcu(). Because the worker does not hold rcu_read_lock(),
synchronize_rcu() will not wait for it.

Doesn't this allow the sibling device to be completely freed while the worker
is still actively dereferencing its memory via hid_get_drvdata(),
wacom_release_resources(), and hid_hw_stop()?

Could using rcu_access_pointer() (which is explicitly intended for pointers
that will not be dereferenced) be papering over the underlying race condition
just to silence sparse/lockdep warnings?

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