Re: [PATCH v4 09/13] HID: asus: add support for response curve

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race in `response_curve_apply` allows the hardware state to desynchronize from the driver's cached sysfs state.

Pre-existing issues:
- [High] The `ally->config` pointer is never initialized to point to the newly allocated configuration struct in `hid_asus_ally_probe()`. This causes `ally_get_config()` to perpetually return `NULL`, breaking all ROG Ally sysfs configuration attributes by making them unconditionally return `-ENODEV`.
--

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

    HID: asus: add support for response curve
    
    ROG ally devices allows configuring the response curve of both joysticks,
    therefore add the ability of userspace to modify the response curve by
    exposing relevant sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index c365b96d4486f..bd488acdb5d49 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1849,6 +1872,400 @@ static struct device_attribute dev_attr_right_trigger_range_upper_limit =
>  static struct device_attribute dev_attr_right_trigger_range_upper_limit_range =
>  	__ATTR(range_upper_limit_range, 0444, right_trigger_range_upper_limit_range_show, NULL);
>  
> +enum ally_joystick_side {
> +	JOYSTICK_LEFT = 0,
> +	JOYSTICK_RIGHT,
> +};
> +
> +/**
> + * ally_set_joystick_resp_curve() - Set joystick response curve parameters
> + * @ally: ally handheld structure
> + * @hdev: HID device
> + * @side: which joystick side (0=left, 1=right)
> + * @curve: response curve parameter structure
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int ally_set_joystick_resp_curve(struct ally_handheld *ally,
> +					struct hid_device *hdev, enum ally_joystick_side side,
> +					struct ally_joystick_resp_curve *curve)
> +{
[ ... ]
> +}
> +
> +static int response_curve_apply(struct ally_handheld *ally,
> +				struct hid_device *hdev,
> +				struct ally_config *cfg, bool is_left)
> +{
> +	struct ally_joystick_resp_curve curve;
> +	int ret;
> +
> +	/*
> +	 * Snapshot under the lock so a concurrent sysfs write cannot change an
> +	 * entry between the monotonicity check and the packet being built.
> +	 */
> +	scoped_guard(mutex, &cfg->config_mutex) {
> +		if (!cfg->resp_curve_support)
> +			return -EOPNOTSUPP;
> +
> +		curve = is_left ? cfg->left_curve : cfg->right_curve;
> +	}
> +
> +	if (!(curve.entry_1.move < curve.entry_2.move &&
> +	      curve.entry_2.move < curve.entry_3.move &&
> +	      curve.entry_3.move < curve.entry_4.move))
> +		return -EINVAL;
> +
> +	ret = ally_set_joystick_resp_curve(ally, hdev,
> +					   is_left ? JOYSTICK_LEFT : JOYSTICK_RIGHT,
> +					   &curve);

[Severity: Medium]
Does dropping config_mutex before calling ally_set_joystick_resp_curve()
introduce a race condition?

If a concurrent sysfs write updates the curve between the scoped_guard() block
and the transmission, could another thread transmit its updated curve first,
causing the hardware to receive the stale snapshot last and leaving it out of
sync with the updated sysfs cache?

[ ... ]


[Severity: High]
This isn't a bug introduced by this patch, but is the config pointer
ever initialized to the newly allocated configuration struct in
hid_asus_ally_probe()?

Looking at hid_asus_ally_probe():

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

Because the ally_cfg returned by ally_config_create() is never assigned to
the handheld's config pointer here, won't ally_get_config() perpetually
return NULL and make all ROG Ally sysfs configuration attributes
unconditionally fail with -ENODEV?

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