Re: [PATCH 10/12] HID: asus: add support for gamepad mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing `hid_asus_ally_remove()` in `asus_probe()` error path leads to Use-After-Free.
- [High] The dynamically allocated `cfg->config_mutex` is never initialized.
- [Medium] User-configured gamepad mode is unconditionally overwritten and lost during device reset/resume.
- [Medium] Hardware state desynchronization due to missing lock coverage over hardware I/O in `gamepad_mode_store`.
- [Medium] Lockless modification of shared state `cfg->gamepad_mode` in `ally_set_default_gamepad_mode()`.
--

--- Patch [10]: [PATCH 10/12] HID: asus: add support for gamepad mode ---
commit cfd28807a39c51dcfa8825ce30540ae3214394d4
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for gamepad mode

ROG Ally devices can emulate either a mouse+keyboard (desktop mode)
or an gamepad device (xbox360 controller in ROG ally and a custom
DInput device on newer models): add support for switching the current
controller mode.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 301fe33f66b1f..8930a246e7255 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -793,6 +807,136 @@ static ssize_t xbox_controller_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(xbox_controller);
>  
> +/**
> + * ally_set_gamepad_mode - Set the gamepad operating mode

[ ... ]

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

[ ... ]

> +	/* Convert the index of the text mode array to the byte
> +	 * that will be accepted by the ally MCU.
> +	 */
> +	mode_byte = ally_gamepad_mode[mode];
> +
> +	ret = ally_set_gamepad_mode(ally, hdev, mode_byte);
> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		cfg->gamepad_mode = mode_byte;

[Severity: Medium]
Can the hardware state become desynchronized from the software state here?

The hardware request ally_set_gamepad_mode() executes before acquiring
cfg->config_mutex, while the internal software state cfg->gamepad_mode
is updated only after the lock is acquired. 

If two threads write to the sysfs attribute concurrently, might their
hardware I/O requests interleave with the mutex-protected state updates,
resulting in the hardware being set to one mode while the driver records
the other?

[Severity: High]
Is cfg->config_mutex properly initialized before it is used here?

Looking at ally_config_create(), the structure is allocated and
zero-initialized:

drivers/hid/hid-asus.c:ally_config_create() {
    ...
	cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
	if (!cfg)
		return ERR_PTR(-ENOMEM);
    ...
}

However, mutex_init() is never called for cfg->config_mutex.

A zero-initialized mutex lacks proper lockdep linkage and wait_list
initialization. 

Could this trigger lockdep warnings or kernel crashes when scoped_guard()
is called?

> +
> +	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
> +
> +	return count;
> +}
> +

[ ... ]

> +static DEVICE_ATTR_RW(gamepad_mode);
> +static DEVICE_ATTR_RO(gamepad_mode_index);
> +
> +static int ally_set_default_gamepad_mode(struct hid_device *hdev,
> +					 struct ally_handheld *ally,
> +					 struct ally_config *cfg)
> +{
> +	cfg->gamepad_mode = ALLY_GAMEPAD_MODE_GAMEPAD;
> +
> +	return ally_set_gamepad_mode(ally, hdev, cfg->gamepad_mode);

[Severity: Medium]
Is it safe to modify cfg->gamepad_mode directly without holding
cfg->config_mutex?

This function is called from hid_asus_ally_init() after ally_config_create()
has already successfully registered the sysfs group. 

Since the sysfs attributes are active and exposed, could a concurrent write
from userspace interleave with this initialization?

> +}
> +
>  /**
>   * ally_set_vibration_intensity() - Set vibration intensity values

[ ... ]

> @@ -2422,6 +2568,13 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  	if (ret < 0)
>  		hid_err(hdev, "Ally failed to init force-feedback off: %d\n", ret);
>  
> +	/* Set the default gamepad mode now that the MCU is confirmed ready */
> +	if (ally->config) {
> +		ret = ally_set_default_gamepad_mode(hdev, ally, ally->config);
> +		if (ret < 0)
> +			hid_warn(hdev, "Failed to set default gamepad mode: %d\n", ret);
> +	}
> +

[Severity: Medium]
Will this unconditionally overwrite the user-configured gamepad mode?

During a USB reset resume, hid_asus_ally_init() is called and invokes
ally_set_default_gamepad_mode(), which sets the state to
ALLY_GAMEPAD_MODE_GAMEPAD and writes it to the hardware. 

Does this actively destroy the user's previously selected mode across
suspend/resume or device reset cycles?

>  	return 0;
>  }

[Severity: Critical]
Does the error path in asus_probe() leave a dangling pointer to freed memory?

If asus_probe() fails after hid_asus_ally_probe() succeeds, the driver
jumps to the err_stop_hw error path:

drivers/hid/hid-asus.c:asus_probe() {
    ...
err_stop_hw:
	if (drvdata->listener.brightness_set)
		asus_hid_unregister_listener(&drvdata->listener);

	asus_worker_stop(drvdata->worker);
	hid_hw_stop(hdev);
	return ret;
}

This path cleans up core resources but fails to tear down the ally
sub-driver state. 

The driver core's devres mechanism automatically frees ally_cfg, but the 
static global ally_drvdata.config retains a dangling pointer to this freed
memory.

Could this lead to a use-after-free if accessed by a subsequent interface
probe or a delayed work?

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