Re: [PATCH 03/12] HID: asus: add gamepad configuration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dangling global pointer `ally_drvdata.cfg_hdev` on probe failure.
- [High] Uninitialized mutex `cfg->config_mutex`.
- [High] TOCTOU NULL pointer dereference in sysfs callbacks reading `ally->config`.
- [Medium] Missing lock acquisitions in sysfs store callback (`xbox_controller_store`).
- [Low] Mixing `goto`-based error handling with the `__free()` scope-based cleanup helper in `ally_check_capability()`.

Pre-existing issues:
- [High] Sleeping in atomic context by acquiring a mutex in HID `raw_event` callback.
--

--- Patch [3]: [PATCH 03/12] HID: asus: add gamepad configuration ---
commit 4090231155d8c981f8463cdfdb5cbc51349b174f
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add gamepad configuration

Add the base configuration structures for the gamepad configuration,
detect capabilities and initialize the device in a known state.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f4b2c4a616b75..6a15380488efb 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -556,6 +590,219 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
>  	return hidbuf;
>  }
>  
> +/**
> + * ally_check_capability - Check if a specific capability is supported
> + * @hdev: HID device
> + * @ally: ally handheld structure
> + * @check_cmd: Capability command code to query
> + *
> + * Returns true if capability is supported, false otherwise
> + */
> +static bool ally_check_capability(struct hid_device *hdev, struct ally_handheld *ally,
> +				  enum ally_command_codes check_cmd)
> +{
> +	u8 payload[] = { 0x00 };
> +	bool result = false;
> +	int ret;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(check_cmd, payload, sizeof(payload));
> +	if (!buf) {
> +		hid_err(hdev, "Failed to allocate buffer for capability check.\n");
> +		goto ally_check_capability_err;

[Severity: Low]
Does this code mix goto-based error handling with the __free() scope-based 
cleanup helper? The cleanup guidelines suggest that goto and cleanup helpers 
should never be mixed in the same function to avoid confusing ownership 
semantics.

> +	}
> +
> +	ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);

[ ... ]

> +static int ally_set_xbox_controller(struct hid_device *hdev,
> +				    struct ally_config *cfg, bool enabled)
> +{
> +	u8 payload[] = { enabled ? 0x01 : 0x00 };
> +	int ret;
> +
> +	if (!cfg || !cfg->xbox_controller_support)
> +		return -ENODEV;
> +
> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_XBOX_CONTROLLER, payload, sizeof(payload));
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = ally_dev_set_report(hdev, buf, ROG_ALLY_REPORT_SIZE);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to set Xbox controller mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	cfg->xbox_controller_enabled = enabled;

[Severity: Medium]
Are we modifying the configuration state and interacting with the hardware 
without acquiring the required locks? The sysfs handler modifies 
cfg->xbox_controller_enabled and sends device reports, but neither 
cfg->config_mutex nor ally->intf_mutex appear to be held here. Could this 
cause a data race if userspace writes to the xbox_controller sysfs attribute?

> +	return 0;
> +}
> +
> +static ssize_t xbox_controller_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;
> +	struct ally_config *cfg;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;

[Severity: High]
Is it possible for ally->config to be freed between the check and this 
assignment? If hid_asus_ally_remove() runs concurrently and sets 
ally->config = NULL, could this result in a NULL pointer dereference?

> +	if (!cfg->xbox_controller_support)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%d\n", cfg->xbox_controller_enabled ? 1 : 0);
> +}
> +
> +static ssize_t xbox_controller_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;
> +	struct ally_config *cfg;
> +	bool enabled;
> +	int ret;
> +
> +	if (!ally || !ally->config)
> +		return -ENODEV;
> +
> +	cfg = ally->config;

[Severity: High]
Like in xbox_controller_show(), could a concurrent device unbind cause a 
NULL pointer dereference here if ally->config is cleared after the check?

> +	if (!cfg->xbox_controller_support)
> +		return -ENODEV;

[ ... ]

> +static struct ally_config *ally_config_create(struct hid_device *hdev, struct ally_handheld *ally)
> +{
> +	struct ally_config *cfg;
> +	int ret, sysfs_i;
> +
> +	cfg = devm_kzalloc(&hdev->dev, sizeof(*cfg), GFP_KERNEL);
> +	if (!cfg)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = ally_detect_capabilities(hdev, ally, cfg);

[Severity: High]
Are we missing an initialization for cfg->config_mutex? The config_mutex in 
struct ally_config is allocated with devm_kzalloc, but it doesn't seem to be 
initialized with mutex_init() before ally_detect_capabilities() uses 
scoped_guard() to lock it. Could this lead to a lockdep splat or panic?

> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to detect Ally capabilities: %d\n", ret);
> +		goto ally_config_create_err;
> +	}

[ ... ]

> @@ -838,8 +1079,30 @@ static struct ally_handheld *hid_asus_ally_probe(struct hid_device *hdev)
>  		switch (ep) {
>  		case HID_ALLY_INTF_CFG_IN:
>  			ally_drvdata.cfg_hdev = hdev;
> +
> +			/*
> +			 * This function assumes the asus-specific initialization
> +			 * to have been performed already at this point.
> +			 */
> +			ret = ally_gamepad_check_ready(&ally_drvdata, hdev);
> +			if (ret < 0) {
> +				hid_err(hdev, "ROG Ally device is not ready: %d\n", ret);
> +				return ERR_PTR(ret);

[Severity: High]
If ally_gamepad_check_ready() fails, do we leave ally_drvdata.cfg_hdev 
pointing to the hid_device? Since ally_drvdata is global state and the device 
will be freed on probe failure, could this cause a use-after-free later?

> +			}
> +

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed something concerning in 
handle_ally_event() when it is called from the raw_event path:

drivers/hid/hid-asus.c:handle_ally_event() {
	...
		scoped_guard(mutex, &ally_data_mutex) {
			keyboard_input = ally->keyboard_input;
			if (keyboard_input) {
	...
}

Since handle_ally_event() is called from the HID raw_event callback, it 
runs in an atomic context where sleeping is prohibited. Could acquiring 
ally_data_mutex here lead to a kernel panic by sleeping in atomic context?

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