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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Use-After-Free (UAF) in sysfs store functions due to manual lockless deallocation of button mappings.
- [High] Sysfs attributes are exposed before `gamepad_mode` initialization, causing a reproducible NULL pointer dereference.
- [Medium] Hardware update errors are silently swallowed for non-Ally X devices, and the software cache is permanently corrupted on failure.
- [Medium] Button remapping sysfs attributes are entirely inaccessible for devices lacking turbo support.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `button_remap_store` bypasses validation.
--

--- Patch [12]: [PATCH 12/12] HID: asus: add support for btn remapping ---
commit d94c33db2e53bcd63e5e7044bbdf330bf4916ffa
Author: Denis Benato <[email protected]>
Subject: HID: asus: add support for btn remapping

Add support for buttons remapping: the ability to assign different
button events to each one of the physical button on the device.

[ ... ]

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 359832395afec..5b744dba98d3e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -2528,29 +3311,78 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
> +static ssize_t button_remap_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
> +	struct ally_config *cfg;
> +	enum ally_button_id button_id = btn_attr->button_id;
> +	enum btn_pair_index pair_idx;
> +	bool is_first;
> +	struct button_pair_map *pair;
> +	struct button_map *btn_map;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;
> +
> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&cfg->config_mutex);
> +	pair = &((struct ally_button_mapping
> +			  *)(cfg->button_mappings))[cfg->gamepad_mode]
> +			.button_pairs[pair_idx - 1];
> +	btn_map = is_first ? &pair->first : &pair->second;
> +
> +	if (btn_attr->is_macro) {
> +		if (btn_map->macro->type == BTN_TYPE_NONE)

[Severity: High]
Could btn_map->macro be NULL here during device initialization?

When ally_create_button_attributes() registers the sysfs groups,
cfg->gamepad_mode is 0 by default. It is not set to 1 (Gamepad) until later
in the probe sequence.

If a user reads the remap or macro sysfs files before cfg->gamepad_mode is
updated, this will index cfg->button_mappings[0]. Since only indexes 1 and 2
are populated by ally_set_default_gamepad_mapping() and
ally_set_default_keyboard_mapping(), the pointers in index 0 remain
uninitialized. Dereferencing btn_map->macro->type would then cause a kernel
panic.

> +			return sysfs_emit(buf, "NONE\n");
> +		else
> +			return sysfs_emit(buf, "%s\n", btn_map->macro->name);
> +	} else {
> +		if (btn_map->remap->type == BTN_TYPE_NONE)
> +			return sysfs_emit(buf, "NONE\n");
> +		else
> +			return sysfs_emit(buf, "%s\n", btn_map->remap->name);
> +	}
> +}

[ ... ]

> +static ssize_t button_remap_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{

[ ... ]

> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Validate that the remap target is allowed for this button/mode */
> +	if (!ally_remap_code_valid(button_id, cfg->gamepad_mode, code))
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		/* Access the mapping for current gamepad mode */
> +		pair = &((struct ally_button_mapping
> +				  *)(cfg->button_mappings))[cfg->gamepad_mode]

[Severity: Medium]
Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here?

The validation function ally_remap_code_valid() reads cfg->gamepad_mode
locklessly. If a concurrent sysfs write to gamepad_mode_store() alters the
mode right before cfg->config_mutex is acquired, an invalid mapping could
bypass the checks and be written to the new mode's configuration.

> +				.button_pairs[pair_idx - 1];
> +		btn_map = is_first ? &pair->first : &pair->second;
> +
> +		if (btn_attr->is_macro)
> +			btn_map->macro = (struct btn_code_map *)code;
> +		else
> +			btn_map->remap = (struct btn_code_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 (cfg->is_ally_x) {
> +			ret = ally_set_button_mapping(hdev, ally, pair);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			mode_map = &((struct ally_button_mapping
> +					 *)(cfg->button_mappings))[cfg->gamepad_mode];
> +			for (i = 0; i < 9; i++) {
> +				mode_map->button_pairs[i].pair_index = i + 1;
> +				ret = ally_set_button_mapping(hdev, ally,
> +					&mode_map->button_pairs[i]);
> +				if (ret < 0)
> +					break;
> +			}
> +		}
> +	}
> +
> +	return count;
> +}

[Severity: Medium]
For non-Ally X devices, if ally_set_button_mapping() fails, this loop breaks
but then unconditionally returns count (success) instead of ret.

Also, earlier in this block, the software cache is updated:

    if (btn_attr->is_macro)
        btn_map->macro = (struct btn_code_map *)code;
    else
        btn_map->remap = (struct btn_code_map *)code;

If the hardware update fails, does this mean the software cache is now
permanently desynchronized from the actual hardware state?

[ ... ]

> @@ -2528,29 +3311,78 @@ static struct ally_btn_turbo_attr *ally_btn_turbo_attr_create(struct hid_device
>  static int ally_create_button_attributes(struct hid_device *hdev, struct ally_config *cfg)
>  {
>  	struct ally_btn_sysfs_entry *entries;
> +	struct ally_button_mapping *mappings;
>  	int i, ret;
>  
> -	if (!cfg->turbo_support)
> -		return 0;
> -

[Severity: Medium]
Even though this check is removed here, does the caller (ally_config_create())
still restrict the creation of these attributes based on cfg->turbo_support?

If ally_create_button_attributes() is only called when cfg->turbo_support is
true, then devices without turbo support will not have access to the button
remapping sysfs attributes either.

>  	entries = devm_kcalloc(&hdev->dev, ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);

[ ... ]

> @@ -2598,8 +3439,16 @@ static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_c
>  	for (i = 0; i < ALLY_BTN_MAX; i++) {
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>  		kfree(entries[i].turbo_attr);
> +		kfree(entries[i].remap_attr);
> +		kfree(entries[i].macro_attr);
> +	}
> +
> +	if (cfg->button_mappings) {
> +		devm_kfree(&hdev->dev, cfg->button_mappings);
> +		cfg->button_mappings = NULL;
>  	}

[Severity: High]
Can this cause a use-after-free if sysfs files are accessed concurrently?

The teardown path frees cfg->button_mappings manually using devm_kfree()
without holding cfg->config_mutex.

If a user writes to gamepad_mode_store() or button_remap_store() right as
this is being freed, it could lead to memory corruption or a kernel panic,
as those functions dereference cfg->button_mappings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12