Re: [PATCH v6 03/13] HID: asus: add support for ROG Ally handhelds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Race condition during concurrent interface probing allows duplicate or malicious USB interfaces to bypass the shared state guard, corrupting global static state.
--

commit 3d735f3f44f1f1940a53b7227c0f6a85b111f6f3
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for ROG Ally handhelds

This commit introduces support for Asus ROG Ally handhelds by extending
the hid-asus driver. It manages a shared global state across multiple USB
interfaces (gamepad, keyboard, configuration) and handles specific input
mapping and initialization sequences for these devices.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 2125680c7effc..11dea2aec80b1 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> +static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
> +{
[ ... ]
> +	spin_lock_irqsave(&ally_data_lock, flags);
> +	if (ally_drvdata.udev && ally_drvdata.udev != udev) {
> +		spin_unlock_irqrestore(&ally_data_lock, flags);
> +		hid_err(hdev, "A ROG Ally controller is already connected\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	/*
> +	 * Interfaces of an already connected controller are equally
> +	 * untrusted: a crafted descriptor could present the same
> +	 * endpoint twice, and a second probe of one role would evict
> +	 * the state recorded by the first one, whose input_dev would
> +	 * then never be unregistered.
> +	 */
> +	if ((ep == HID_ALLY_INTF_CFG_IN && ally_drvdata.cfg_hdev) ||
> +	    (ep == HID_ALLY_X_INTF_IN && ally_drvdata.ally_x_hdev) ||
> +	    (ep == HID_ALLY_INTF_KEYBOARD_IN && ally_drvdata.keyboard_hdev)) {
> +		spin_unlock_irqrestore(&ally_data_lock, flags);
> +		hid_err(hdev,
> +			"An interface of a ROG Ally controller is already connected\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	if (!ally_drvdata.udev) {
> +		usb_get_dev(udev);
> +		ally_drvdata.udev = udev;
> +	}
> +	spin_unlock_irqrestore(&ally_data_lock, flags);
> +
> +	/*
> +	 * The interface initialization sleeps (it performs USB transfers),
> +	 * so it must run before taking the spinlock guarding the shared
> +	 * state; pointers are published only once it succeeded.
> +	 */
> +	switch (ep) {
> +	case HID_ALLY_INTF_CFG_IN:
> +		/*
> +		 * This function assumes the asus-specific initialization
> +		 * to have been performed already at this point.
> +		 */
> +		ret = ally_gamepad_check_ready(&ally_drvdata, hdev);
> +		if (ret < 0) {
> +			hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
> +			ally_put_udev_if_orphaned();
> +			return ERR_PTR(ret);
> +		}
> +
> +		ret = hid_asus_ally_init(hdev, &ally_drvdata);
> +		if (ret < 0) {
> +			ally_put_udev_if_orphaned();
> +			return ERR_PTR(ret);
> +		}
> +
> +		spin_lock_irqsave(&ally_data_lock, flags);
> +		ally_drvdata.cfg_hdev = hdev;
> +		spin_unlock_irqrestore(&ally_data_lock, flags);

[Severity: Medium]
Is there a race condition during concurrent interface probing here?

The ally_data_lock is dropped after validating that the interface hasn't
been probed yet (e.g., ally_drvdata.cfg_hdev is NULL), allowing the thread
to perform sleepable USB I/O.

During this window, a concurrent probe from a duplicate or malicious USB
interface could also acquire ally_data_lock, see that ally_drvdata.cfg_hdev
is still NULL, and proceed with its own sleepable setup.

If multiple threads bypass this check and one fails during initialization,
it might call ally_put_udev_if_orphaned() and drop the udev reference
prematurely because the other thread has not yet set ally_drvdata.cfg_hdev.

If multiple succeed, they could sequentially overwrite the shared pointer,
leaking resources and corrupting the shared global state.

Could we reserve the interface slot or track a probe-in-progress state
under the lock before dropping it to wait on the sleepable setup?

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