Re: [PATCH 1/1] HID: wacom: serialize mode changes with device removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: wacom: Use-After-Free due to sibling worker restarting hardware during remove
--

commit d345d10fbdd75c5608a4db1f5c93eaec7ccc124e
Author: Jinmo Yang <jinmo44.yang@gmail.com>
Subject: HID: wacom: serialize mode changes with device removal

This commit adds a driver-wide wacom_mode_change_lock mutex to serialize
mode-change workers with wacom_remove(). The goal is to prevent a sibling
device's mode-change worker from accessing a removed device's struct wacom
after it has been freed.

Fixes: 4082da80f46a ("HID: wacom: generic: add mode change touch key")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2783,22 +2784,32 @@ static void wacom_remote_work(struct work_struct *work)
>  static void wacom_mode_change_work(struct work_struct *work)
>  {
[ ... ]
> +	if (pen) {
> +		wacom1 = hid_get_drvdata(pen);
>  		wacom_release_resources(wacom1);
>  		hid_hw_stop(wacom1->hdev);

[Severity: High]
If a sibling worker is running concurrently with wacom_remove(), could this
double-stop the hardware of the device being removed?

[ ... ]
>  	if (wacom1) {
>  		error = wacom_parse_and_register(wacom1, false);
>  		if (error)
> -			return;
> +			goto out;
>  	}

[Severity: High]
Could the sibling worker restart the hardware via wacom_parse_and_register()
(which calls hid_hw_start()) here, while wacom_remove() is blocked waiting
on the wacom_mode_change_lock?

[ ... ]
> @@ -2916,6 +2928,10 @@ static void wacom_remove(struct hid_device *hdev)
>  	cancel_work_sync(&wacom->battery_work);
>  	cancel_work_sync(&wacom->remote_work);
>  	cancel_work_sync(&wacom->mode_change_work);
> +
> +	/* A sibling's mode-change work can also access this device. */
> +	mutex_lock(&wacom_mode_change_lock);

[Severity: High]
Since wacom_remove() executes teardown steps like hid_hw_stop() before
acquiring the lock, and only cancels its own worker, could a sibling's
worker continue running and bypass these teardown steps?

[ ... ]
>  	if (wacom->wacom_wac.features.type != REMOTE)
>  		wacom_release_resources(wacom);
> +
> +	mutex_unlock(&wacom_mode_change_lock);
>  }

[Severity: High]
If the sibling's worker restarted the hardware via wacom_parse_and_register(),
wacom_remove() returns here without calling hid_hw_stop() again.

Will this leave the device's hidraw nodes active and registered after the
driver is unbound, leading to a Use-After-Free of struct wacom when accessed
by userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004111353.118025-1-jinmo44.yang@gmail.com?part=1