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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] HID: asus: inverted sysfs_streq() check in find_button_by_name()
- [Medium] HID: asus: inconsistent pair update and rollback in button_remap_store()
- [Low] HID: asus: mixed cleanup helpers and goto in button_remap_store()
--

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

HID: asus: add support for btn remapping

This commit adds sysfs support for remapping physical buttons and assigning
macro codes on Asus ROG Ally devices, updating the cached mappings when the
gamepad mode changes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 70c9d71fc5fa9..dcb3c9f5d23e8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -978,10 +1186,587 @@ static struct ally_prefs {
>  	struct ally_joystick_resp_curve right_curve;
>  
>  	struct ally_turbo_config turbo;
> +	struct ally_button_mapping button_mappings;
>  } ally_prefs = {
>  	.lock = __MUTEX_INITIALIZER(ally_prefs.lock),
>  };
>  
> +/**
> + * find_button_by_name() - Find a button code map by its name
> + * @name: name of the button code to look up
> + *
> + * This function deals with data from userspace therefore it
> + * must not trust the input blindly.
> + *
> + * Return: the matching button code map, or NULL if not found.
> + */
> +static const struct btn_code_map *find_button_by_name(const char *name)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ally_btn_codes); i++) {
> +		if (!sysfs_streq(ally_btn_codes[i].name, name))
> +			return &ally_btn_codes[i];
> +	}
> +
> +	return NULL;
> +}

[Severity: High]
Is the sysfs_streq() condition in find_button_by_name() inverted?

Unlike strcmp(), sysfs_streq() returns true when the strings match. Because
ally_btn_codes[0] is "NONE" and ally_btn_codes[1] is "BTN_SOUTH", negating
sysfs_streq() causes find_button_by_name("NONE") in
ally_set_button_pair_mappings_default() to return &ally_btn_codes[1]
(BTN_SOUTH), and causes lookups for any other button name (or invalid strings
written by userspace to button_remap_store()) to immediately return
&ally_btn_codes[0] (NONE).

When hid_asus_ally_init() calls ally_apply_button_mappings(), or when
gamepad_mode_store() or button_remap_store() updates mappings, all default
remaps resolve to NONE, all default macros resolve to BTN_SOUTH, and invalid
button names from userspace never fail with -EINVAL.

[ ... ]

> +static ssize_t button_remap_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
> +	enum ally_button_id button_id = btn_attr->button_id;
> +	const struct btn_code_map *old_code, *code;
> +	const struct btn_code_map **target_map;
> +	struct ally_button_mapping *mode_map;
> +	enum btn_pair_index pair_idx;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	bool is_first;
> +	int ret, i;
> +
> +	code = find_button_by_name(buf);
> +	if (!code)
> +		return -EINVAL;
> +
> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&ally_prefs.lock);
> +
> +	/*
> +	 * Validate the remap target while holding the lock: the
> +	 * gamepad mode can change concurrently, and the check must
> +	 * be done against the very mode the mapping is written to.
> +	 */
> +	if (!ally_remap_code_valid(button_id, ally_prefs.gamepad_mode, code,
> +				   btn_attr->is_macro))
> +		return -EINVAL;
> +
> +	/* Access the mapping for current gamepad mode */
> +	pair = &ally_prefs.button_mappings.button_pairs[pair_idx - 1];
> +	btn_map = is_first ? &pair->first : &pair->second;
> +
> +	target_map = btn_attr->is_macro ? &btn_map->macro : &btn_map->remap;
> +	old_code = *target_map;
> +	*target_map = code;
> +
> +	/* Update pair index */
> +	pair->pair_index = pair_idx;
> +
> +	/*
> +	 * Send mapping to device with the caveat that first
> +	 * generation devices require individual button pair updates.
> +	 */
> +	if (ally_prefs.is_ally_x) {
> +		ret = ally_set_button_mapping(hdev, pair);
> +		if (ret < 0)
> +			goto err_restore;
> +	} else {
> +		mode_map = &ally_prefs.button_mappings;
> +		for (i = 0; i < ARRAY_SIZE(mode_map->button_pairs); i++) {
> +			mode_map->button_pairs[i].pair_index = i + 1;
> +			ret = ally_set_button_mapping(hdev,
> +						      &mode_map->button_pairs[i]);
> +			if (ret < 0) {
> +				/*
> +				 * Pairs are pushed one at a time. This pair
> +				 * already reached the hardware when a later
> +				 * pair is the one that failed.
> +				 */
> +				if (i > pair_idx - 1)
> +					goto err_restore_hw;
> +				goto err_restore;
> +			}
> +		}
> +	}
> +	sysfs_notify(&dev->kobj, ally_button_names[btn_attr->button_id],
> +		     btn_attr->is_macro ? "macro" : "remap");
> +	return count;
> +
> +err_restore_hw:
> +	*target_map = old_code;
> +
> +	/* Try to recover the mapping presented to the user. */
> +	ally_set_button_mapping(hdev, pair);
> +	goto err_out;
> +
> +err_restore:
> +	*target_map = old_code;
> +
> +err_out:
> +	hid_err(hdev, "Failed to set button mapping: %d\n", ret);
> +	return ret;
> +}

[Severity: Medium]
Does the err_restore_hw path in button_remap_store() properly roll back the
hardware state on first-generation ROG Ally devices?

In ally_detect_capabilities(), ally_prefs.is_ally_x is set only for the second-
generation ROG Ally X (USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X). In
button_remap_store(), the comment says first-generation devices require
individual button pair updates, yet the code sends a single pair when
ally_prefs.is_ally_x is true and loops over all 9 pairs in
mode_map->button_pairs when ally_prefs.is_ally_x is false.

Furthermore, err_restore_hw is only reachable from the !ally_prefs.is_ally_x
loop when ally_set_button_mapping() fails at iteration i > pair_idx - 1, but
err_restore_hw then calls ally_set_button_mapping(hdev, pair) for only that
single pair. If !ally_prefs.is_ally_x devices require all 9 pairs to be sent,
wouldn't a single-pair update in err_restore_hw fail to restore the hardware
mapping, or if single-pair updates work on !ally_prefs.is_ally_x, why does the
else branch send all 9 pairs?

[ ... ]

> @@ -2399,11 +3200,11 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>  
>  static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_device *hdev)
>  {
> +	size_t entries_attr_index;
>  	int i, ret;
>  
>  	scoped_guard(mutex, &ally_prefs.lock)
> -		if (!ally_prefs.turbo_support)
> -			return NULL;
> +		entries_attr_index = ally_prefs.turbo_support ? 4 : 0;
>  
>  	struct ally_btn_sysfs_entry *entries __free(kfree) = kzalloc_objs(*entries, ALLY_BTN_MAX);
>  	if (!entries)
> @@ -2412,13 +3213,33 @@ static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_dev
>  	for (i = 0; i < ALLY_BTN_MAX; i++) {
>  		entries[i].btn = i;
>  
> -		if (!ally_btn_param(i)) {
> -			hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> -			ret = -EINVAL;
> +		if (ally_prefs.turbo_support) {
> +			if (!ally_btn_param(i)) {
> +				hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
> +				ret = -EINVAL;
> +				goto ally_btn_err_cleanup;
> +			}
> +
> +			ally_btn_turbo_init_attrs(&entries[i]);
> +		}
> +
> +		entries[i].remap_attr = button_remap_attr_create(i, false);
> +		entries[i].macro_attr = button_remap_attr_create(i, true);
> +		if (!entries[i].remap_attr || !entries[i].macro_attr) {
> +			ret = -ENOMEM;
>  			goto ally_btn_err_cleanup;
>  		}
>  
> -		ally_btn_turbo_init_attrs(&entries[i]);
> +		sysfs_attr_init(&entries[i].attr_remap_available.attr);
> +		entries[i].attr_remap_available.attr.name = "remap_available";
> +		entries[i].attr_remap_available.attr.mode = 0444;
> +		entries[i].attr_remap_available.show = btn_remap_available_show;
> +
> +		entries[i].attrs[entries_attr_index + 0] = &entries[i].remap_attr->dev_attr.attr;
> +		entries[i].attrs[entries_attr_index + 1] = &entries[i].macro_attr->dev_attr.attr;
> +		entries[i].attrs[entries_attr_index + 2] = &entries[i].attr_remap_available.attr;
> +		entries[i].attrs[entries_attr_index + 3] = NULL;
> +
>  		entries[i].group.name = ally_button_names[i];
>  		entries[i].group.attrs = entries[i].attrs;
>  
> @@ -2436,6 +3257,11 @@ static struct ally_btn_sysfs_entry *ally_create_button_attributes(struct hid_dev
>  	while (i--)
>  		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);
> +	}
> +
>  	return ERR_PTR(ret);
>  }

[Severity: Low]
As reported by Sashiko on v12 [1], this isn't a bug, but button_remap_store()
mixes guard(mutex)(&ally_prefs.lock) with goto labels (err_restore_hw,
err_restore, and err_out) while also holding ally_prefs.lock across
sysfs_notify(), and ally_create_button_attributes() similarly mixes
scoped_guard() and __free(kfree) on entries with goto ally_btn_err_cleanup.

Could button_remap_store() and ally_create_button_attributes() avoid mixing
scope-based cleanup helpers with goto error paths (and could remap_attr and
macro_attr be embedded directly in struct ally_btn_sysfs_entry like
attr_turbo_period, attr_toggle_period, and attr_remap_available)?

[1] https://lore.kernel.org/r/sashiko-outbox-192087@kernel.org

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