Re: [PATCH 08/12] 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:
- [Critical] Missing NULL pointer checks in sysfs callbacks cause a reliable kernel panic during device probe (NULL pointer dereference).
--

--- Patch [8]: [PATCH 08/12] HID: asus: add support for response curve ---
commit 6ba0f4853fce7434b3f1730f265395260f85991d
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: add support for response curve

This commit adds support for configuring the joystick response curves on ROG
ally devices. It introduces several sysfs attributes that allow userspace to
modify the response curve parameters.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8ed10c5b22ab5..8018e61cf0264 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1584,6 +1607,319 @@ static struct device_attribute dev_attr_right_trigger_range_upper_limit =
>  static struct device_attribute dev_attr_right_trigger_range_upper_limit_range =
>  	__ATTR(range_upper_limit_range, 0444, right_trigger_range_upper_limit_range_show, NULL);
>  
> +enum ally_joystick_side {
> +	JOYSTICK_LEFT = 0,
> +	JOYSTICK_RIGHT,
> +};
> +
[ ... ]
> +static int response_curve_apply(struct hid_device *hdev, bool is_left)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	struct ally_handheld *const ally = drvdata->rog_ally;
> +	struct ally_config *cfg = ally->config;

[Severity: Critical]
Could ally->config become NULL here if the device is concurrently unbound?

The check for !ally || !ally->config happens in
left_response_curve_apply_store and right_response_curve_apply_store before
calling this function, but it is performed outside of any lock. Could this
lead to a NULL pointer dereference if the state changes in between?

> +	struct ally_joystick_resp_curve curve;
> +	int ret;
> +
> +	/*
> +	 * Snapshot under the lock so a concurrent sysfs write cannot change an
> +	 * entry between the monotonicity check and the packet being built.
> +	 */
> +	scoped_guard(mutex, &cfg->config_mutex)
> +		curve = is_left ? cfg->left_curve : cfg->right_curve;
> +
[ ... ]
> +#define DEFINE_JS_CURVE_PCT_FOPS(region, side)					\
> +	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);\
> +	}									\

[Severity: Critical]
Can ally or ally->config be NULL here when userspace (like udev) reads
these sysfs attributes during device probe?

If they are accessed before drvdata->rog_ally is fully populated, would
&ally->config->side##_curve cause a NULL pointer dereference? The older
sysfs attributes appear to include an if (!ally || !ally->config) check
before dereferencing these.

> +										\
> +	static ssize_t side##_response_curve_pct_##region##_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 *ally = drvdata->rog_ally;			\
> +		return response_curve_pct_store(dev, attr, buf, count,		\
> +						side##_is_left, ally, region);	\
> +	}
> +
> +#define DEFINE_JS_CURVE_MOVE_FOPS(region, side)					\
> +	static ssize_t side##_response_curve_move_##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_move_show(					\
> +			dev, attr, buf, &ally->config->side##_curve, region);\
> +	}									\

[Severity: Critical]
Does this also need a check to prevent a NULL pointer dereference if ally or
ally->config are accessed during probe?

> +										\
> +	static ssize_t side##_response_curve_move_##region##_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 *ally = drvdata->rog_ally;			\
> +		return response_curve_move_store(dev, attr, buf, count,	        \
> +						 side##_is_left, ally, region); \
> +	}

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