Re: [PATCH 05/12] HID: asus: add joysticks inner and outer range configuration

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized Mutex `config_mutex` in `ally_config`
- [High] NULL Pointer Dereference due to TOCTOU on `ally->config`
- [Medium] Data Race and State Desynchronization in Sysfs Handlers
- [Low] Dead Code: Unused Macros
--

--- Patch [5]: [PATCH 05/12] HID: asus: add joysticks inner and outer range configuration ---
commit bd4086a93a46d9dd4b9c0379eb086b2a077f1489
Author: Denis Benato <[email protected]>

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 48cc867aeea86..010b33533068a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -133,6 +133,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  
>  #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
>  
> +#define ALLY_DEVICE_ATTR_RO(_name, _sysfs_name)    \
> +	struct device_attribute dev_attr_##_name = \
> +		__ATTR(_sysfs_name, 0444, _name##_show, NULL)
> +
> +#define ALLY_DEVICE_CONST_ATTR_RO(fname, sysfs_name, value)			\
> +	static ssize_t fname##_show(struct device *dev,				\
> +				   struct device_attribute *attr, char *buf)	\
> +	{									\
> +		return sysfs_emit(buf, value);					\
> +	}									\
> +	struct device_attribute dev_attr_##fname =				\
> +		__ATTR(sysfs_name, 0444, fname##_show, NULL)

[Severity: Low]
Are these macros used anywhere in this patch or the rest of the codebase?

[ ... ]

> +static ssize_t left_joystick_inner_threshold_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_joystick_thresholds(hdev, ally->config,
> +					   value,
> +					   ally->config->left_outer_threshold,
> +					   ally->config->right_deadzone,
> +					   ally->config->right_outer_threshold);

[Severity: High]
Could ally->config become NULL after the initial check if a concurrent device
removal (like USB unplug) occurs while this sysfs write is executing?

If hid_asus_ally_remove() runs concurrently and sets ally->config to NULL:

hid_asus_ally_remove() {
    if (ally->cfg_hdev == hdev) {
        ally_config_remove(hdev, ally);
        ally->cfg_hdev = NULL;
        ally->config = NULL;
    }
}

Then this function could dereference a NULL pointer when reading
ally->config->left_outer_threshold. Would it be safer to cache the config
pointer locally?

[Severity: Medium]
Does reading ally->config->left_outer_threshold and other variables
locklessly create a race condition with other concurrent sysfs writes?

If two writes interleave, they could both read the same threshold values
before either updates the hardware, causing the hardware state to become
desynchronized with the sysfs cache. Should the mutex be acquired before
reading the current configuration and sending the hardware command?

> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)

[Severity: High]
Is config_mutex initialized before being used here?

Looking at ally_config_create(), the configuration structure is allocated
with devm_kzalloc(), but mutex_init() is never called on config_mutex. Will
acquiring this all-zeros mutex trigger a crash or lockdep failure?

> +		ally->config->left_deadzone = value;
> +
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5