Re: [PATCH v11 11/18] 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:
- [Medium] HID: asus: race publishing button_entries in ally_create_button_attributes()
--

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

HID: asus: add support for btn remapping

This commit adds sysfs support for remapping physical buttons and macros on
ROG Ally handheld devices, including per-button remap_index attributes and
automatic cache refreshes when the gamepad mode changes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index ed95b2cdec813..ea27495c3df66 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1141,6 +1480,33 @@ static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *a
>  
>  	cfg->gamepad_mode = mode_byte;
>  
> +	/*
> +	 * When the gamepad mode changes, the hardware automatically
> +	 * resets all button mappings to its internal defaults for the
> +	 * new mode.
> +	 *
> +	 * Refresh the software cache so that subsequent
> +	 * reads from sysfs reflect what the firmware is actually
> +	 * using.
> +	 */
> +	if (mode_byte == ALLY_GAMEPAD_MODE_GAMEPAD)
> +		ally_set_default_gamepad_mapping(&cfg->button_mappings);
> +	else if (mode_byte == ALLY_GAMEPAD_MODE_KEYBOARD)
> +		ally_set_default_keyboard_mapping(&cfg->button_mappings);
> +
> +	/*
> +	 * The re-defaulted mappings are what the remap and macro files
> +	 * report: wake anyone polling them about the new values.
> +	 */
> +	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");
> +		}
> +	}
[ ... ]
> @@ -2824,15 +3759,12 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>   * 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_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;
> @@ -2845,13 +3777,53 @@ static int ally_create_button_attributes(struct hid_device *hdev,
>  		entries[i].hdev = hdev;
>  		entries[i].btn = i;
>  
> -		if (!ally_btn_get_turbo_params(cfg, i)) {
> -			hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> -			ret = -EINVAL;
> +		if (cfg->turbo_support) {
> +			if (!ally_btn_get_turbo_params(cfg, i)) {
> +				hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> +				ret = -EINVAL;
> +				goto err_cleanup;
> +			}
> +
> +			ally_btn_turbo_init_attrs(&entries[i]);
> +		}
> +
> +		entries[i].remap_attr = button_remap_attr_create(ally, i, false);
> +		if (!entries[i].remap_attr) {
> +			ret = -ENOMEM;
>  			goto err_cleanup;
>  		}
>  
> -		ally_btn_turbo_init_attrs(&entries[i]);
> +		entries[i].macro_attr = button_remap_attr_create(ally, i, true);
> +		if (!entries[i].macro_attr) {
> +			ret = -ENOMEM;
> +			goto err_cleanup;
> +		}
> +
> +		/* Initialize the remap_index attribute */
> +		sysfs_attr_init(&entries[i].attr_remap_index.attr);
> +		entries[i].attr_remap_index.attr.name = "remap_index";
> +		entries[i].attr_remap_index.attr.mode = 0444;
> +		entries[i].attr_remap_index.show = btn_remap_index_show;
> +		entries[i].attr_remap_index.store = NULL;
> +
> +		/* Set up attributes array based on what's supported */
> +		if (cfg->turbo_support) {
> +			entries[i].attrs[4] =
> +				&entries[i].remap_attr->dev_attr.attr;
> +			entries[i].attrs[5] =
> +				&entries[i].macro_attr->dev_attr.attr;
> +			entries[i].attrs[6] =
> +				&entries[i].attr_remap_index.attr;
> +			entries[i].attrs[7] = NULL;
> +		} else {
> +			entries[i].attrs[0] =
> +				&entries[i].remap_attr->dev_attr.attr;
> +			entries[i].attrs[1] =
> +				&entries[i].macro_attr->dev_attr.attr;
> +			entries[i].attrs[2] =
> +				&entries[i].attr_remap_index.attr;
> +			entries[i].attrs[3] = NULL;
> +		}
>  
>  		entries[i].group.name = ally_button_names[i];
>  		entries[i].group.attrs = entries[i].attrs;
> @@ -2867,12 +3839,27 @@ static int ally_create_button_attributes(struct hid_device *hdev,
>  	return 0;
>  
>  err_cleanup:
> +	/*
> +	 * Unpublish first: the readers of button_entries (like the
> +	 * gamepad_mode_store() cross-notification) hold config_mutex and
> +	 * must observe either the live array or NULL, never a freed one.
> +	 * The lock is released again before the removals, so a straggler
> +	 * callback waiting on it can drain against the not yet freed
> +	 * entries.
> +	 */
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		cfg->button_entries = NULL;
> +	}

[Severity: Medium]
Can gamepad_mode_store() observe a partially initialized cfg->button_entries
array while ally_create_button_attributes() is still running?

In hid_asus_ally_probe() -> ally_config_create(), ally->config is published
and ally_attr_groups[0] (which exposes gamepad_mode) is registered with
sysfs_create_group() before ally_create_button_attributes() is called:

drivers/hid/hid-asus.c:ally_config_create() {
	...
	spin_lock_irqsave(&ally_data_lock, flags);
	ally->config = cfg;
	spin_unlock_irqrestore(&ally_data_lock, flags);

	for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_attr_groups); sysfs_i++) {
		ret = sysfs_create_group(&hdev->dev.kobj,
					 &ally_attr_groups[sysfs_i]);
	...
	ret = ally_create_button_attributes(hdev, ally, cfg);
}

At the start of ally_create_button_attributes(), cfg->button_entries = entries
is assigned without holding cfg->config_mutex and before the loop initializes
entries[i].group.name and creates the sysfs groups:

drivers/hid/hid-asus.c:ally_create_button_attributes() {
	entries = kcalloc(ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
	if (!entries)
		return -ENOMEM;

	cfg->button_entries = entries;

	for (i = 0; i < ALLY_BTN_MAX; i++) {
		...
		entries[i].group.name = ally_button_names[i];
		...
}

If userspace writes to gamepad_mode while this loop is running (or before
err_cleanup acquires cfg->config_mutex on failure), gamepad_mode_store()
acquires cfg->config_mutex, sees a non-NULL cfg->button_entries, and iterates
over all ALLY_BTN_MAX entries, racing with the unlocked writes to
entries[i].group.name and passing NULL as the directory name to
sysfs_notify() for entries that have not been initialized yet.

Should cfg->button_entries = entries instead be published under
cfg->config_mutex only after all ALLY_BTN_MAX groups have been initialized
and created?

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