Re: [PATCH v4 06/13] HID: asus: add joysticks inner and outer range configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Hardware state desynchronization across suspend/resume: joystick thresholds are not restored to the MCU.
- [High] Missing sysfs group removal in `ally_config_remove()` allows concurrent hardware I/O during device unbind, leading to UAF.

Pre-existing issues:
- [High] Missing assignment of ally->config makes all sysfs attributes unconditionally fail with -ENODEV.
--

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

    HID: asus: add joysticks inner and outer range configuration
    
    ROG Ally devices supports configuring joysticks inner and outer range:
    add sysfs attributes to allow userspace modifying the sensitivity
    of those controllers.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f825d650e1b7d..7702427bde451 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -973,6 +973,351 @@ static struct device_attribute dev_attr_right_vibration_intensity =
>  static struct device_attribute dev_attr_right_vibration_intensity_range =
>  	__ATTR(intensity_range, 0444, right_vibration_intensity_range_show, NULL);
>  
[ ... ]
> +static ssize_t left_joystick_inner_threshold_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 *const ally = drvdata->rog_ally;
> +	struct ally_config *cfg;
> +
> +	if (!ally)
> +		return -ENODEV;
> +
> +	cfg = ally_get_config(ally);
> +	if (!cfg)
> +		return -ENODEV;

[Severity: High]
This isn't a bug introduced by this patch, but will these new sysfs attributes
unconditionally fail with -ENODEV?

Looking at hid_asus_ally_probe(), the ally_cfg pointer allocated by
ally_config_create() is never assigned to ally_drvdata.config:

    spin_lock_irqsave(&ally_data_lock, flags);
    ally_drvdata.cfg_hdev = hdev;
    spin_unlock_irqrestore(&ally_data_lock, flags);

Because ally_drvdata.config is missing, won't ally_get_config(ally) always
return NULL here and cause every new sysfs callback to fail immediately?

> +
> +	guard(mutex)(&cfg->config_mutex);
> +
> +	if (!cfg->user_cal_support)
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "%u\n", cfg->left_deadzone);
> +}
[ ... ]
> @@ -1028,6 +1411,19 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  		goto ally_config_create_err;
>  	}
>  
> +	/* Skip the calibration groups when the capability is missing. */
> +	if (cfg->user_cal_support) {
> +		for (sysfs_i = 0; sysfs_i < ARRAY_SIZE(ally_cal_attr_groups); sysfs_i++) {
> +			ret = devm_device_add_group(&hdev->dev,
> +						    ally_cal_attr_groups[sysfs_i]);

[Severity: High]
Can concurrent sysfs writes during device unbind cause a use-after-free by
racing with hid_hw_stop()?

The driver manually calls sysfs_remove_group() for its older attributes in
ally_config_remove() to ensure active callbacks drain completely before
hardware teardown:

    for (i = 0; i < ARRAY_SIZE(ally_attr_groups); i++)
        sysfs_remove_group(&hdev->dev.kobj, &ally_attr_groups[i]);

Since ally_cal_attr_groups is not added to this manual removal loop, won't the
new sysfs store callbacks fail to drain? This could allow them to call
hid_hw_raw_request() concurrently while hid_hw_stop() is dismantling the
transport.

> +			if (ret < 0) {
> +				hid_err(hdev, "Failed to create sysfs group '%s': %d\n",
> +					ally_cal_attr_groups[sysfs_i]->name, ret);
> +				goto ally_config_create_sysfs_err;
> +			}
> +		}
> +	}
> +
>  	cfg->gamepad_mode = 0x01;
>  	cfg->left_deadzone = 10;
>  	cfg->left_outer_threshold = 90;

[Severity: High]
Will the hardware state for joystick thresholds become desynchronized after a
suspend/resume cycle?

The MCU loses its state upon reset/suspend, so hid_asus_ally_init() is
explicitly invoked on resume to restore the software-cached state:

    /* The MCU may have just been reset: restore the cached state. */
    guard(mutex)(&cfg->config_mutex);

    if (cfg->xbox_controller_enabled) {
        ret = ally_set_xbox_controller(hdev, ally, cfg, true);
        ...
    }

Since hid_asus_ally_init() completely omits restoring the new joystick
threshold parameters added in this patch, won't the hardware operate with
factory defaults while sysfs falsely reports the user's customized values?

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