Re: [PATCH v2 11/12] HID: asus: add support for turbo buttons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Sysfs attribute leak on probe error path leads to a Use-After-Free when accessed.
- [High] Calling `sysfs_remove_group()` while holding a spinlock with interrupts disabled causes a scheduling-while-atomic kernel panic.
- [Medium] State desynchronization on write failure: the local cached turbo configuration is permanently modified before the hardware acknowledges the change.
- [Low] Missing trailing return statement after `scoped_guard` causes `-Wreturn-type` compiler warnings.
--

--- Patch [11]: [PATCH v2 11/12] HID: asus: add support for turbo buttons ---
commit 1c542d232085f0c0b17c85a45a979b053ef8c90e
Author: Denis Benato <denis.benato@linux.dev>

    HID: asus: add support for turbo buttons
    
    ROG ally devices supports a feature called turbo buttons that allows
    the user to quickly toggle button state by keeping a button pressed,
    therefore allow userspace to configure the feature by exposing
    relevant sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6be25cc022cba..d6ab906b7e782 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static ssize_t btn_turbo_period_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
> +							  attr_turbo_period);
> +	struct ally_btn_turbo_params *params = ally_btn_get_turbo_params(entry->cfg,
> +									 entry->btn);
> +
> +	if (!params)
> +		return -ENODEV;
> +
> +	scoped_guard(mutex, &entry->cfg->config_mutex)
> +		return sysfs_emit(buf, "%hhu\n", params->turbo);
> +}

[Severity: Low]
Does this code trigger a -Wreturn-type compiler warning?

The function ends inside a scoped_guard() block without an explicit trailing
return statement. Since scoped_guard() is implemented as a macro wrapping a for
loop, the compiler's flow analysis cannot definitively prove the loop executes,
which leads to a warning that control reaches the end of a non-void function.

> +
> +static ssize_t btn_turbo_period_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
> +							  attr_turbo_period);
> +	struct ally_btn_turbo_params *params;
> +	u8 value;
> +	int ret;
> +
> +	if (!entry->cfg->turbo_support)
> +		return -EOPNOTSUPP;
> +
> +	params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
> +	if (!params)
> +		return -ENODEV;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (value < ALLY_TURBO_PERIOD_MIN || value > ALLY_TURBO_PERIOD_MAX)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &entry->cfg->config_mutex) {
> +		params->turbo = value;
> +		ret = ally_set_turbo_params(entry->ally, entry->hdev, entry->cfg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return count;
> +}

[Severity: Medium]
Are we permanently desynchronizing local state if the hardware transmission fails?

By updating params->turbo before confirming the hardware acknowledged the change
via ally_set_turbo_params(), the local cache becomes modified. If the USB/HID
transmission fails, the function returns an error without rolling back
params->turbo. 

A subsequent successful sysfs write to another button will then silently flush
this invalid cached state to the hardware.

> +
> +static ssize_t btn_toggle_period_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
> +							  attr_toggle_period);
> +	struct ally_btn_turbo_params *params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
> +
> +	if (!params)
> +		return -ENODEV;
> +
> +	scoped_guard(mutex, &entry->cfg->config_mutex)
> +		return sysfs_emit(buf, "%hhu\n", params->toggle);
> +}

[Severity: Low]
Does this code trigger a -Wreturn-type compiler warning?

The function ends inside a scoped_guard() block without an explicit trailing
return statement. Since scoped_guard() is implemented as a macro wrapping a for
loop, the compiler's flow analysis cannot definitively prove the loop executes,
which leads to a warning that control reaches the end of a non-void function.

> +
> +static ssize_t btn_toggle_period_store(struct device *dev, struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	struct ally_btn_sysfs_entry *entry = container_of(attr, struct ally_btn_sysfs_entry,
> +							  attr_toggle_period);
> +	struct ally_btn_turbo_params *params;
> +	u8 value;
> +	int ret;
> +
> +	if (!entry->cfg->turbo_support)
> +		return -EOPNOTSUPP;
> +
> +	params = ally_btn_get_turbo_params(entry->cfg, entry->btn);
> +	if (!params)
> +		return -ENODEV;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (value < ALLY_TOGGLE_PERIOD_MIN || value > ALLY_TOGGLE_PERIOD_MAX)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &entry->cfg->config_mutex) {
> +		params->toggle = value;
> +		ret = ally_set_turbo_params(entry->ally, entry->hdev, entry->cfg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return count;
> +}

[Severity: Medium]
Are we permanently desynchronizing local state if the hardware transmission fails?

By updating params->toggle before confirming the hardware acknowledged the change
via ally_set_turbo_params(), the local cache becomes modified. If the USB/HID
transmission fails, the function returns an error without rolling back
params->toggle. 

A subsequent successful sysfs write to another button will then silently flush
this invalid cached state to the hardware.

[ ... ]

> @@ -2556,6 +2967,9 @@ static void ally_config_remove(struct hid_device *hdev, struct ally_handheld *al
>  
>  	if (!cfg || !cfg->initialized)
>  		return;
> +
> +	if (cfg->turbo_support && cfg->button_entries)
> +		ally_remove_button_attributes(hdev, cfg);
>  }

[Severity: High]
Does this cause a scheduling-while-atomic kernel panic during device unbind?

Looking at hid_asus_ally_remove(), it acquires ally_data_lock via
spin_lock_irqsave() and disables interrupts before calling ally_config_remove():

drivers/hid/hid-asus.c:hid_asus_ally_remove() {
	...
	spin_lock_irqsave(&ally_data_lock, flags);
	...
	if (ally->cfg_hdev == hdev) {
		ally_config_remove(hdev, ally);
	...
}

This patch modifies ally_config_remove() to call ally_remove_button_attributes()
which calls sysfs_remove_group(). Because sysfs_remove_group() blocks on
kernfs_rwsem, sleeping inside this atomic context will predictably trigger a
panic when the device is unbound or the module is removed.

[Severity: Critical]
Does this result in a Use-After-Free and sysfs attribute leak if probe fails?

When hid_asus_ally_setup() calls hid_asus_ally_init(), an error causes
ally_config_remove() to be called:

drivers/hid/hid-asus.c:hid_asus_ally_setup() {
	...
		ret = hid_asus_ally_init(hdev, &ally_drvdata);
		if (ret < 0) {
			ally_config_remove(hdev, &ally_drvdata);
			return ERR_PTR(ret);
		}

		spin_lock_irqsave(&ally_data_lock, flags);
		ally_drvdata.config = ally_cfg;
	...
}

However, ally_drvdata.config is not assigned until after this check. When
ally_config_remove() runs, ally_drvdata.config is NULL, causing it to return
early without calling ally_remove_button_attributes(). 

The sysfs groups created by ally_create_button_attributes() are leaked and 
remain accessible. When devres subsequently frees the backing memory
(ally_cfg and entries), any unprivileged user reading the leaked sysfs files
like btn_turbo_period_show() will dereference freed memory, triggering a UAF.

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