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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unvalidated and unapplied response curve staging state is written directly to the hardware during device resume or initialization.
--

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

HID: asus: add support for response curve

This commit adds support for configuring the response curve of the joysticks
on ROG Ally devices by exposing sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index cddb72973bae7..5c3749a914b7f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1867,6 +1890,396 @@ static struct device_attribute dev_attr_right_trigger_range_upper_limit =
> +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;
> +
> +	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;

[ ... ]

> +static ssize_t response_curve_move_store(struct device *dev,
> +					 const char *buf, size_t count,
> +					 struct ally_config *cfg, bool is_left,
> +					 unsigned int idx)
> +{
> +	struct ally_joystick_resp_curve *curve;
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (value > 100)
> +		return -EINVAL;
> +
> +	guard(mutex)(&cfg->config_mutex);
> +
> +	if (!cfg->resp_curve_support)
> +		return -EOPNOTSUPP;
> +
> +	curve = is_left ? &cfg->left_curve : &cfg->right_curve;
> +
> +	switch (idx) {
> +	case 1:
> +		curve->entry_1.move = value;
> +		break;

[Severity: Medium]
Since this modifies the shared configuration state without validating it
against other curve entries, is it possible for the curve to be left in an
invalid, non-monotonic state?

If a user writes invalid values without successfully calling
response_curve_apply(), this unvalidated state remains in cfg->left_curve.

[ ... ]

> @@ -2425,6 +2876,22 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>  					 ret);
>  	}
>  
> +	if (cfg->resp_curve_support) {
> +		ret = ally_set_joystick_resp_curve(ally, hdev, JOYSTICK_LEFT,
> +							   &cfg->left_curve);

[Severity: Medium]
Could this unconditional push to the hardware during initialization or resume
send an invalid response curve to the MCU?

Because response_curve_move_store() directly modifies cfg->left_curve
without ensuring the entries are strictly monotonic, this path bypasses the
validation checks present in response_curve_apply().

Sending unvalidated hardware state could result in unexpected device
behavior or garbage joystick data.

Would it be better to stage the sysfs changes in a separate buffer, or
validate the curve here before writing it to the device?

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