Re: [PATCH 07/12] HID: asus: add joysticks anti-deadzone configuration

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

New issues:
- [Medium] Concurrent sysfs writes to left and right anti-deadzone attributes can desynchronize the hardware state from the software state due to a Time-of-Check to Time-of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] The `config_mutex` inside `struct ally_config` is used without being initialized.
- [High] Sysfs attribute callbacks can dereference a NULL `ally->config` pointer if a concurrent device removal occurs.
--

--- Patch [7]: [PATCH 07/12] HID: asus: add joysticks anti-deadzone configuration ---
commit d78f09e927da53af9bf7a52457ad40ae819eb0ce
Author: Denis Benato <[email protected]>

HID: asus: add joysticks anti-deadzone configuration

ROG Ally devices allow configuring the anti-deadzone parameter for
the resistive joysticks devices as over time those develops drift.
This commit exposes relevant sysfs attributes to allow userspace to
configure the anti-deadzone.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index eb735ec8f065c..8ed10c5b22ab5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1155,6 +1155,165 @@ static struct device_attribute dev_attr_right_joystick_outer_threshold =
>  static struct device_attribute dev_attr_right_joystick_outer_threshold_range =
>  	__ATTR(outer_threshold_range, 0444, right_joystick_outer_threshold_range_show, NULL);
>  
> +/**
> + * ally_set_anti_deadzone - Set anti-deadzone values for joysticks
> + * @hdev: HID device
> + * @left_adz: Left joystick anti-deadzone value (0-100)
> + * @right_adz: Right joystick anti-deadzone value (0-100)
> + *
> + * Return: 0 on success, negative on failure
> + */
> +static int ally_set_anti_deadzone(struct hid_device *hdev, u8 left_adz, u8 right_adz)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	const u8 payload[] = { left_adz, right_adz };
> +	int ret;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_ANTI_DEADZONE, payload, sizeof(payload));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (!ally->config->anti_deadzone_support) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but could this dereference a NULL pointer?

If a concurrent device removal occurs, the remove() callback explicitly sets
ally->config = NULL. Since the sysfs groups are managed by devres, they are
removed after the remove() callback completes, leaving a race window.

A concurrent sysfs access could pass the initial check in
left_joystick_anti_deadzone_store(), get interrupted while the remove path
nullifies ally->config, and then crash here when ally_set_anti_deadzone()
directly dereferences it.

> +		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to set anti-deadzone values: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static ssize_t left_joystick_anti_deadzone_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;
> +
> +	if (!ally->config->anti_deadzone_support) {
> +		hid_dbg(hdev, "Anti-deadzone not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = kstrtou8(buf, 10, &value);
> +	if (ret || value > 100)
> +		return -EINVAL;
> +
> +	ret = ally_set_anti_deadzone(hdev, value, ally->config->right_anti_deadzone);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &ally->config->config_mutex)
> +		ally->config->left_anti_deadzone = value;

[Severity: Medium]
Could this result in a Time-of-Check to Time-of-Use (TOCTOU) race condition?

If two threads concurrently write to the left and right anti-deadzone
attributes, they could locklessly read the opposite joystick's state from
ally->config, send conflicting commands to the hardware, and then sequentially
acquire config_mutex to update the software cache. Depending on which hardware
request reaches the device last, the hardware might drop one of the updates,
but the software cache would reflect both new values, causing a permanent
desynchronization.

[Severity: High]
This is a pre-existing issue, but I noticed config_mutex inside
struct ally_config is used here without being initialized.

Looking at ally_config_create(), it is allocated with devm_kzalloc():

    cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
    if (!cfg)
        return ERR_PTR(-ENOMEM);

Because the memory is zeroed but mutex_init() is never called, the wait
queue list head (wait_list) is not initialized. Could this cause a kernel
crash in the mutex slow path if there is contention or if
CONFIG_DEBUG_MUTEXES is enabled?

> +
> +	return count;
> +}

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