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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Memory leak of dynamically allocated `button_entries` and sysfs groups on `hid_asus_ally_init()` failure.
--

commit bb828c60d81e9c63c13fe68b948f5a447a8d5424
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 60228dc84b50d..5cdbcc2bd1bc7 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2579,6 +2675,339 @@ static const struct attribute_group *const ally_cal_attr_groups[] = {

[ ... ]

> +/**
> + * ally_create_button_attributes() - Create turbo button attributes
> + * @hdev: HID device
> + * @ally: ally handheld structure
> + * @cfg: ally config structure
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int ally_create_button_attributes(struct hid_device *hdev,
> +					 struct ally_handheld *ally,
> +					 struct ally_config *cfg)
> +{
> +	struct ally_btn_sysfs_entry *entries;
> +	int i, ret;
> +
> +	if (!cfg->turbo_support)
> +		return 0;
> +
> +	entries = kcalloc(ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
> +	if (!entries)
> +		return -ENOMEM;
> +
> +	cfg->button_entries = entries;

[Severity: High]
If hid_asus_ally_init() fails later during probe, does this code leak the
newly allocated button_entries and sysfs groups?

Looking at hid_asus_ally_probe(), if hid_asus_ally_init() fails, it returns
an error without calling ally_config_remove() to roll back the configuration:

	ally_cfg = ally_config_create(hdev, &ally_drvdata);
	if (IS_ERR(ally_cfg)) {
		...
	}

	ret = hid_asus_ally_init(hdev, &ally_drvdata);
	if (ret < 0) {
		ally_put_udev_if_orphaned();
		return ERR_PTR(ret);
	}

Because ally_config is global, a transient USB error causing a probe failure
might lead to the next probe re-executing ally_create_button_attributes().
Would this overwrite the cfg->button_entries pointer and permanently leak the
prior kcalloc array, as well as leave stale sysfs attributes attached that
cause subsequent probes to fail with -EEXIST?

> +
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {

[ ... ]

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