Re: [PATCH v8 12/12] HID: asus: add support for btn remapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: asus: use-after-free of button_entries during teardown and error paths
--

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

HID: asus: add support for btn remapping

This commit adds support for button remapping on ROG Ally devices. It
introduces sysfs attributes to configure valid remap targets and automatically
refreshes the software cache when the gamepad mode changes.

> 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
[ ... ]
> @@ -1131,6 +1470,33 @@ static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *a
[ ... ]
> +	if (cfg->button_entries) {
> +		for (i = 0; i < ALLY_BTN_MAX; i++) {
> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
> +				     "remap");
> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
> +				     "macro");
> +		}
> +	}
[ ... ]
> @@ -2883,12 +3860,22 @@ static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_c
[ ... ]
>  	kfree(entries);
>  
> -	cfg->button_entries = NULL;
> +	/*
> +	 * Unpublish the pointers under the lock, so that a sysfs callback
> +	 * which is still holding config_mutex observes the teardown
> +	 * instead of freed memory.
> +	 */
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		cfg->button_entries = NULL;
> +	}
>  }

[Severity: High]
Can this result in a use-after-free when sysfs callbacks run concurrently?

Here entries (which corresponds to cfg->button_entries) is freed via
kfree(entries) before the pointer is actually set to NULL under
config_mutex. If a concurrent sysfs write to gamepad_mode_store() acquires
config_mutex right after this kfree() but before the scoped_guard executes, 
it will see a non-NULL cfg->button_entries and dereference freed memory when 
accessing cfg->button_entries[i].group.name.

Should cfg->button_entries be set to NULL inside the lock before entries is
freed?

[ ... ]
> @@ -2861,6 +3833,11 @@ static int ally_create_button_attributes(struct hid_device *hdev,
[ ... ]
>  err_cleanup:
>  	while (--i >= 0)
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>  
> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
> +		kfree(entries[i].remap_attr);
> +		kfree(entries[i].macro_attr);
> +	}
> +
>  	kfree(entries);
>  
>  	cfg->button_entries = NULL;

[Severity: High]
Is there a similar race condition on this error path?

If ally_create_button_attributes() fails and takes this error path,
cfg->button_entries is set to NULL without holding config_mutex, and after
entries has already been freed. This seems to expose the same use-after-free
window to any concurrent sysfs accesses.

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