Re: [PATCH 4/5] 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` without RCU read lock leads to Use-After-Free.
- [High] Asynchronous resources started during probe are not cancelled if a subsequent probe step fails.
--

commit cf6bbd224806057956b357da6af1b313f006731c
Author: Lee Jones <lee@kernel.org>

HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
    
This patch resolves a use-after-free issue in wacom_bamboo_pad_pen_event
by introducing RCU protection for the pen and touch pointers.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b88726cbbb4ec..79fd397e044af 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2836,16 +2847,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 is a pre-existing issue, but does using rcu_access_pointer() here leave
the pointer unprotected?

The rcu_access_pointer() helper doesn't establish an RCU read-side critical
section. This means a concurrent removal of the sibling device could call
wacom_remove_shared_data() and nullify the shared pointer.

If the pointer is nullified, synchronize_rcu() won't wait for this work item
to finish.

Could this sequence allow the device and its driver data to be freed while
we're still executing hid_hw_stop() on it, resulting in a use-after-free?

>  		wacom1->wacom_wac.has_mode_change = true;
>  		wacom1->wacom_wac.is_direct_mode = is_direct;
>  	}
[ ... ]

Also, regarding a pre-existing issue in wacom_parse_and_register() not modified
by this patch:

[Severity: High]
This isn't a bug introduced by this patch, but does the error path here fail to
cancel asynchronous works?

When an initialization step fails after hid_hw_start(hdev) is called,
the cleanup goes to fail_hw_stop:

fail_hw_stop:
	hid_hw_stop(hdev);
fail:
	wacom_release_resources(wacom);
	return error;

The call to hid_hw_start() enables interrupts, which could schedule work items
like wacom->battery_work, wacom->wireless_work, or wacom->mode_change_work.

Does failing to cancel these scheduled works before returning allow them to
execute after devres has freed the wacom structure?

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