Re: [PATCH v9 09/12] HID: asus: add support to force feedback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: asus: UAF in hid_asus_ally_init due to missing gamepad disconnect re-check
--

commit b25c2490b184182f7d805afd2134e7bf9ce60b63
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: add support to force feedback

This commit adds support for force feedback to the Asus ROG Ally X gamepad,
using a work-queue to avoid sleeping calls in atomic contexts when sending
XInput rumble magnitude updates.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index bb664bd9e140b..298f14b8713d9 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -2679,20 +2846,61 @@ static int ally_x_setup_input(struct hid_device *hdev, struct ally_handheld *all
[ ... ]
>  static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *ally)
>  {
> +	struct hid_device *x_hdev;
>  	struct ally_config *cfg;
> +	unsigned long flags;
>  	int ret;
>  
[ ... ]
> +	/*
> +	 * Snapshot the gamepad interface and send the force-feedback "off"
> +	 * packet through it. usbhid_stop() frees urbout before NULLing it
> +	 * while usbhid_output_report() dereferences it unlocked, so a send
> +	 * overlapping the transport teardown would be a use-after-free: the
> +	 * device lock is taken around the send because the HID core holds
> +	 * it across remove() and the hid_hw_stop() that follows, making the
> +	 * two mutually exclusive. It is taken before intf_mutex, the same
> +	 * order the core's remove path uses, so no lock inversion exists.
> +	 */
> +	spin_lock_irqsave(&ally_data_lock, flags);
> +	x_hdev = ally->ally_x_hdev;
> +	if (x_hdev)
> +		get_device(&x_hdev->dev);
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	if (x_hdev) {
> +		/* Failure at this point is non-critical */
> +		scoped_guard(mutex, &x_hdev->dev.mutex) {
> +			scoped_guard(mutex, &ally->intf_mutex) {
> +				ret = ally_x_send_ff_off(x_hdev);

[Severity: High]
Can this sequence lead to a use-after-free if the device is concurrently
unplugged?

If hid_asus_ally_init() snapshots x_hdev and takes a reference to it, but the
device is unplugged before the mutex is acquired, the HID core will run the
unbind process (which takes x_hdev->dev.mutex, calls remove(), and then calls
hid_hw_stop()). 

During this unbind, remove() safely clears ally->ally_x_hdev under
ally->intf_mutex, and hid_hw_stop() stops the transport and frees the
low-level driver data (e.g. kfree(usbhid)).

Once unbind completes, hid_asus_ally_init() acquires x_hdev->dev.mutex and
ally->intf_mutex. Because it does not re-verify if ally->ally_x_hdev == x_hdev
under the lock, it proceeds to call ally_x_send_ff_off(x_hdev) on the
torn-down transport. This forwards the request to hid_hw_output_report(),
which will dereference the freed low-level driver data, causing a
use-after-free.

Would it be safer to check if ally->ally_x_hdev is still equal to x_hdev
inside the intf_mutex guard before sending the report?

> +
> +				if (ret < 0)
> +					hid_err(hdev,
> +						"Ally failed to init force-feedback off: %d\n",
> +						ret);
> +			}
> +		}
> +
> +		put_device(&x_hdev->dev);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009122820.7603-1-denis.benato@linux.dev?part=9