Re: [PATCH v4 12/13] HID: asus: add support for turbo buttons

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Use-After-Free of turbo button sysfs entries during driver unbind.
- [Low] Memory leak of devm-allocated `entries` array on sysfs creation error path.

Pre-existing issues:
- [High] Missing assignment of newly created configuration to the global shared state.
--

commit c80fc8c450c40cb3fda087280dc3fa093ed01eeb
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 9ed6c605df888..7fc04d29de8de 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2684,6 +3118,8 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  
>  	return cfg;
>  ally_config_create_sysfs_err:
> +	if (cfg->button_entries)
> +		ally_remove_button_attributes(hdev, cfg);
>  ally_config_create_err:
>  	devm_kfree(&hdev->dev, cfg);
>  	return ERR_PTR(ret);

[Severity: Low]
Does this code leak the entries array if sysfs creation fails?

In ally_create_button_attributes(), the entries array is allocated using
devm_kcalloc(). If sysfs_create_group() fails, the error path sets
cfg->button_entries = NULL.

When returning here, cfg->button_entries is NULL, so
ally_remove_button_attributes() is skipped. The cfg structure is freed
manually to save memory, but the devm-allocated entries array is abandoned.

Since asus_probe() might ignore probe failures from the Ally extension,
the devres group remains alive, causing this memory to stay allocated until
the device is unbound. Could the entries array be freed directly on this
error path?

[ ... ]

> @@ -2701,6 +3137,9 @@ static void ally_config_remove(struct hid_device *hdev, struct ally_config *cfg)
>  	if (!cfg || !cfg->initialized)
>  		return;
>  
> +	if (cfg->button_entries)
> +		ally_remove_button_attributes(hdev, cfg);
> +
>  	for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
>  		sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);
>  }

[Severity: High]
This is a pre-existing issue, but it appears the newly created configuration
in hid_asus_ally_probe() is never assigned to the global shared state.

In hid_asus_ally_probe():

    ally_cfg = ally_config_create(hdev, &ally_drvdata);
    if (IS_ERR(ally_cfg)) {
    ...
    spin_lock_irqsave(&ally_data_lock, flags);
    ally_drvdata.cfg_hdev = hdev;
    // ally_drvdata.config is not set to ally_cfg
    spin_unlock_irqrestore(&ally_data_lock, flags);

Because ally->config is never set, subsequent config lookups fail, and
the cfg parameter passed to ally_config_remove() is always NULL.

[Severity: High]
Can the unbind path cause a use-after-free regression here?

Because cfg is always NULL (due to the pre-existing issue mentioned above),
the early return in ally_config_remove() is taken, and
ally_remove_button_attributes() is skipped during unbind.

Since sysfs_create_group() is used to manually create the sysfs attributes,
they are not tied to devres. However, their backing memory (the entries
array) is allocated with devm_kcalloc() and will be freed by devres.

This leaves the sysfs files active after the memory is freed. If a user
reads one of these leftover sysfs files after device unplug (e.g., in
btn_turbo_period_show()), it dereferences the freed memory, causing a
use-after-free.

Could the missing configuration assignment be fixed so cleanup occurs
correctly, or could the sysfs group lifecycles be tied to devres as well?

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