Re: [PATCH v7 02/12] HID: asus: add support for ROG Ally handhelds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hid-asus: ROG Ally hotplug is permanently broken due to uncleared probe_in_progress flags
- [Medium] hid-asus: TOCTOU race in ally_put_udev_if_orphaned drops USB device reference prematurely
- [Medium] hid-asus: TOCTOU race in keyboard interface publication violates singleton safety
--

commit 88e0fb100cdf5cb20d748a018414785e32e69eed
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for ROG Ally handhelds

This commit adds support for ASUS ROG Ally handhelds to the hid-asus driver. 
It extends the driver to handle multiple HID interfaces (gamepad, config, 
keyboard) using a shared global state structure and implements specific 
input mappings and events.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
>  static void ally_put_udev_if_orphaned(void)
>  {
>  	struct usb_device *udev;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&ally_data_lock, flags);
>  	if (!ally_drvdata.udev || ally_drvdata.keyboard_hdev ||
>  	    ally_drvdata.cfg_hdev || ally_drvdata.ally_x_hdev) {
>  		spin_unlock_irqrestore(&ally_data_lock, flags);
>  		return;
>  	}

[Severity: Medium]
Does this prematurely drop the USB device reference during a concurrent
probe? This condition checks the published pointers but doesn't check the
cfg_probe_in_progress or ally_x_probe_in_progress flags. If one interface
fails its initialization and calls this function while another is currently
sleeping in its probe, it appears the udev reference will be dropped,
breaking the singleton safety check.

[ ... ]

>  static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  {
>  	unsigned long flags;
>  	int ret, ep = ally_get_endpoint_address(hdev);

[ ... ]

>  	if ((ep == HID_ALLY_INTF_CFG_IN &&
>  	     (ally_drvdata.cfg_hdev || ally_drvdata.cfg_probe_in_progress)) ||
>  	    (ep == HID_ALLY_X_INTF_IN &&
>  	     (ally_drvdata.ally_x_hdev || ally_drvdata.ally_x_probe_in_progress)) ||
>  	    (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);
>  	}
>  
>  	/* Reserve the role for as long as the initialization below runs. */
>  	if (ep == HID_ALLY_INTF_CFG_IN)
>  		ally_drvdata.cfg_probe_in_progress = true;
>  	else if (ep == HID_ALLY_X_INTF_IN)
>  		ally_drvdata.ally_x_probe_in_progress = true;

[Severity: High]
These probe_in_progress flags are set to true here to reserve the role, but
looking at the successful completion paths below, they never seem to be set
back to false. Once an interface successfully probes, do these flags remain
permanently stuck at true, causing hotplug attempts to fail?

>  	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 not run under ally_data_lock: the role stays reserved
>  	 * while it runs, and the pointers are published only once it
>  	 * succeeded.
>  	 */
>  	switch (ep) {

[ ... ]

>  	case HID_ALLY_INTF_KEYBOARD_IN:
>  		spin_lock_irqsave(&ally_data_lock, flags);
>  		ally_drvdata.keyboard_hdev = hdev;
>  		if (!list_empty(&hdev->inputs)) {
>  			hidinput = list_first_entry(&hdev->inputs, struct hid_input, list);
>  			ally_drvdata.keyboard_input = hidinput->input;
>  		}
>  		spin_unlock_irqrestore(&ally_data_lock, flags);
>  		break;

[Severity: Medium]
Can a TOCTOU race occur here for the keyboard interface? The lock is dropped
after checking ally_drvdata.keyboard_hdev earlier in this function, and
re-acquired here to assign the pointer. Since the keyboard interface doesn't
use a probe_in_progress reservation, could two concurrent keyboard probes
both see the pointer as NULL, pass the check, and then both overwrite the
shared state here?

[ ... ]

>  static void hid_asus_ally_remove(struct hid_device *hdev, struct ally_handheld *ally)
>  {
>  	struct input_dev *x_input = NULL;
>  	unsigned long flags;
>  
>  	if (!ally)
>  		return;

[ ... ]

>  	spin_lock_irqsave(&ally_data_lock, flags);
>  	if (ally->ally_x_hdev == hdev) {
>  		x_input = ally->ally_x_input;
>  		ally->ally_x_input = NULL;
>  		ally->ally_x_hdev = NULL;
>  	}

[Severity: High]
The hdev pointers are cleared here during removal, but there is no code to
clear the corresponding cfg_probe_in_progress or ally_x_probe_in_progress
flags. Should these flags be reset to false when the device is unbound to
prevent permanently blocking future probes?

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