Re: [PATCH 11/12] HID: asus: add support for turbo buttons

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

New issues:
- [Medium] Lockless read of `cfg->turbo` in `ally_set_turbo_params` leads to data races and hardware state desynchronization.
- [Low] Redundant and completely unused dynamic allocation of `struct ally_btn_turbo_attr`.

Pre-existing issues:
- [High] Preexisting sysfs show/store callbacks for response curves dereference `ally->config` without checking if it is NULL, leading to a kernel crash if accessed during device unbind.
--

--- Patch [11]: [PATCH 11/12] HID: asus: add support for turbo buttons ---
commit 889fd7b803cb63ae392e549855086950d7932992
Author: Denis Benato <[email protected]>

HID: asus: add support for turbo buttons

ROG ally devices supports a feature called turbo buttons that allows
the user to quickly toggle button state by keeping a button pressed,
therefore allow userspace to configure the feature by exposing
relevant sysfs attributes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8930a246e7255..359832395afec 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -2195,6 +2288,321 @@ static const struct attribute_group ally_attr_groups[] = {
> +static int ally_set_turbo_params(struct hid_device *hdev, struct ally_config *cfg)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *ally = drvdata->rog_ally;
> +	struct ally_turbo_config *turbo = &cfg->turbo;
> +	const u8 payload[] = {
> +		turbo->btn_du.turbo,
> +		turbo->btn_du.toggle,

[Severity: Medium]
Can this lockless read result in stale or torn data being sent to the hardware?

In btn_turbo_period_store(), the config_mutex is released immediately before
calling ally_set_turbo_params():

    scoped_guard(mutex, &entry->cfg->config_mutex)
        params->turbo = value;

    ret = ally_set_turbo_params(entry->hdev, entry->cfg);

Since ally_set_turbo_params() locklessly reads the entire turbo array to
construct the payload, concurrent sysfs writes to other buttons could result
in the device receiving an inconsistent or desynchronized state. Could this
be prevented by holding config_mutex while building the payload?

> +		turbo->btn_dd.turbo,
> +		turbo->btn_dd.toggle,
[ ... ]
> +struct ally_btn_turbo_attr {
> +	struct device_attribute dev_attr;
> +	int button_id;
> +};

[Severity: Low]
Does this code allocate a redundant structure?

Later in ally_btn_turbo_attr_create(), this struct is allocated via
kzalloc_obj, but its fields are never populated. The actual sysfs attributes
used seem to be embedded within struct ally_btn_sysfs_entry instead.

> +
> +#define to_ally_btn_turbo_attr(x) container_of(x, struct ally_btn_turbo_attr, dev_attr)
[ ... ]


[Severity: High]
This is a pre-existing issue, but can this code cause a kernel panic if the
sysfs attribute is read during device unbind?

Looking at DEFINE_JS_CURVE_PCT_FOPS in drivers/hid/hid-asus.c:

	static ssize_t side##_response_curve_pct_##region##_show(		\
		struct device *dev, struct device_attribute *attr, char *buf)	\
	{									\
		struct hid_device *hdev = to_hid_device(dev);			\
		struct asus_drvdata *drvdata = hid_get_drvdata(hdev);		\
		struct ally_handheld *ally = drvdata->rog_ally;			\
		return response_curve_pct_show(					\
			dev, attr, buf, &ally->config->side##_curve, region);\
	}

When the device is unplugged, hid_asus_ally_remove() synchronously sets
ally->config = NULL. Since devres handles the sysfs removal later, a
concurrent read of the sysfs file unconditionally dereferences ally->config
without checking if it is NULL.

Should there be a NULL check before evaluating &ally->config->side_curve
in both DEFINE_JS_CURVE_PCT_FOPS and DEFINE_JS_CURVE_MOVE_FOPS?

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