Re: [PATCH v9 12/12] 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: Data race and potential crash due to lockless, early publication of cfg->button_entries
- [Medium] hid-asus: Spurious -ENODEV in btn_remap_index_show during probe window
- [Low] hid-asus: Mixing of goto-based error handling and scope-based cleanup macros
--

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

    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 e9380002e9ffb..c439f0eeedc1d 100644
> --- 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
>  
>  	cfg->gamepad_mode = mode_byte;
>  

[ ... ]

> +	if (cfg->button_entries) {
> +		for (i = 0; i < ALLY_BTN_MAX; i++) {
> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
> +				     "remap");

[Severity: High]
Does this code introduce a data race with ally_create_button_attributes()?

Since ally_config_create() registers the main sysfs attributes (including
gamepad_mode) before calling ally_create_button_attributes(), userspace can
write to gamepad_mode while the button entries are being initialized.

ally_create_button_attributes() assigns the allocated array to
cfg->button_entries without any locks or memory barriers:

ally_create_button_attributes()
	entries = kcalloc(ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
	if (!entries)
		return -ENOMEM;
	cfg->button_entries = entries;

If gamepad_mode_store() runs concurrently, it acquires cfg->config_mutex,
observes the non-NULL cfg->button_entries pointer, and attempts to pass
cfg->button_entries[i].group.name to sysfs_notify().

As the array elements may not yet be initialized, this could pass a NULL or
garbage pointer to sysfs_notify(), potentially leading to a crash in
kernfs_find_and_get().

> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
> +				     "macro");
> +		}
> +	}
> +
>  	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
>  

[ ... ]

> +static ssize_t btn_remap_index_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 ally_btn_sysfs_entry *entry = container_of(attr,
> +		struct ally_btn_sysfs_entry, attr_remap_index);
> +	struct ally_config *cfg;
> +	enum ally_button_id btn = entry->btn;
> +	u8 mode;
> +	ssize_t len = 0;
> +	int i;
> +
> +	if (!ally)
> +		return -ENODEV;

[Severity: Medium]
Will this return a spurious -ENODEV if userspace accesses the sysfs
attribute immediately after creation?

The globally published drvdata->rog_ally pointer is not set until
asus_probe() completes. However, the sysfs files are created and exposed
during hid_asus_ally_probe().

If userspace accesses this file before asus_probe() finishes,
drvdata->rog_ally will be NULL, causing this function to fail with -ENODEV.

Should this code use the safely cached entry->ally pointer instead of
attempting to retrieve the global pointer from drvdata?

> +
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return -ENODEV;

[ ... ]

> @@ -2858,12 +3830,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: Low]
Does this code violate the kernel cleanup guidelines by mixing goto-based
error handling with scope-based cleanups?

The cleanup subsystem documentation in include/linux/cleanup.h mandates
that functions must not mix goto-based cleanup labels with scope-based
cleanups (guard() or scoped_guard()). The rule states to either convert all
resources to scope-based cleanup, or convert none of them.

Since ally_create_button_attributes() relies entirely on goto-based error
handling for unwinding memory and sysfs allocations, could you use standard
mutex_lock() and mutex_unlock() calls here instead?

> +
>  	while (--i >= 0)
>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);

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