Re: [PATCH v6 04/13] HID: asus: add gamepad configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Time-of-check to time-of-use race condition in probe allows spoofed USB devices to bypass singleton checks, leading to memory corruption and UAF.
- [Medium] Sysfs attribute becomes visible before device initialization is complete, causing transient -ENODEV errors.
--

commit 40a018a18704e85c526230c53db4138c9ec858c7
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add gamepad configuration

Add the base configuration structures for the gamepad configuration,
detect capabilities and initialize the device in a known state.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 11dea2aec80b1..ac0c0a176857e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -617,6 +660,274 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
>  	return hidbuf;
>  }

[ ... ]

> +static ssize_t xbox_controller_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +
> +	if (!ally)
> +		return -ENODEV;

[Severity: Medium]
Will userspace configuration scripts encounter transient errors when trying
to read or configure the controller immediately upon detection?

In ally_config_create(), sysfs_create_group() is called, which immediately
exposes attributes like xbox_controller to userspace.

If a read or write occurs immediately, xbox_controller_show() retrieves
drvdata->rog_ally. However, this pointer is only assigned later in
asus_probe() after hid_asus_ally_probe() returns. During this window,
this callback will return -ENODEV.

While this race is handled later in the series for button mapping attributes
by explicitly carrying the ally pointer in a custom attribute structure,
do these base attributes need similar protection?

> +
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return -ENODEV;

[ ... ]

> @@ -1025,6 +1356,14 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  			return ERR_PTR(ret);
>  		}
>  
> +		ally_cfg = ally_config_create(hdev, &ally_drvdata);

[Severity: High]
Can this code allow a time-of-check to time-of-use race if multiple USB
interfaces are probed concurrently?

The singleton check earlier in hid_asus_ally_probe() checks
ally_drvdata.cfg_hdev under ally_data_lock, but the lock is released
before reaching this point.

If two threads probe concurrently, both will pass the initial check. They
would then both execute ally_config_create() concurrently without a lock,
leading to a memory leak of the static ally_config pointer and a double
initialization of its mutex.

Finally, if both threads re-acquire the lock and overwrite
ally_drvdata.cfg_hdev, the device that loses the race would have its sysfs
groups permanently leaked. If the driver is later unloaded, ally_config is
freed, but the leaked sysfs attributes would remain, leading to a
use-after-free on subsequent accesses.

> +		if (IS_ERR(ally_cfg)) {
> +			hid_err(hdev, "Failed to create Ally cfg: %ld\n",
> +				PTR_ERR(ally_cfg));
> +			ally_put_udev_if_orphaned();
> +			return ERR_PTR(PTR_ERR(ally_cfg));
> +		}
> +
>  		ret = hid_asus_ally_init(hdev, &ally_drvdata);

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