Re: [PATCH 06/12] HID: asus: add triggers inner and outer range configuration

[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 condition in trigger threshold sysfs stores causes hardware state desynchronization.

Pre-existing issues:
- [High] The `config_mutex` in `struct ally_config` is dynamically allocated but never explicitly initialized.
--

--- Patch [6]: [PATCH 06/12] HID: asus: add triggers inner and outer range configuration ---
commit bf1ab6fc2d6df4d87a7b2cef0f0cf8628bb2f010
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add triggers inner and outer range configuration

This patch adds sysfs attributes to configure the inner and outer ranges for
the left and right triggers on ROG Ally devices. It implements the store and
show functions for these limits and sends the updated configuration to the
hardware.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 010b33533068a..eb735ec8f065c 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> +static ssize_t left_trigger_range_lower_limit_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	u8 value;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 50)
> +		return -EINVAL;
> +
> +	ret = ally_set_trigger_ranges(hdev, ally->config,
> +					   value,
> +					   ally->config->left_trigger_max,
> +					   ally->config->right_trigger_min,
> +					   ally->config->right_trigger_max);

[Severity: Medium]
Could this cause a TOCTOU data race if multiple threads write to the trigger
sysfs attributes concurrently?

Since the other limit values are read locklessly from `ally->config` before the
hardware is updated, concurrent writes to different attributes could interleave.

This might result in one thread sending a stale limit while overwriting the
other thread's hardware update, permanently desynchronizing the hardware
configuration from the driver's software cache.

Should the mutex be acquired before calling `ally_set_trigger_ranges()`?

> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)
> +		ally->config->left_trigger_min = value;
> +
> +	return count;
> +}

[ ... ]

> @@ -1246,6 +1540,10 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>  	cfg->left_outer_threshold = 90;
>  	cfg->right_deadzone = 10;
>  	cfg->right_outer_threshold = 90;
> +	cfg->left_trigger_min = 0;
> +	cfg->left_trigger_max = 100;
> +	cfg->right_trigger_min = 0;
> +	cfg->right_trigger_max = 100;

[Severity: High]
This is a pre-existing issue, but is `config_mutex` ever explicitly initialized?

In `ally_config_create()`, `cfg` is dynamically allocated via `devm_kzalloc()`
which zero-initializes the memory.  However, it appears
`mutex_init(&cfg->config_mutex)` is never called before the mutex is locked in
functions like `ally_detect_capabilities()` or the sysfs store operations.

Could this lead to a kernel panic or lockdep warning when the uninitialized
mutex is locked under contention?

>  	cfg->vibration_intensity_left = 100;
>  	cfg->vibration_intensity_right = 100;
>  	cfg->vibration_active = false;

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