Re: [PATCH] HID: ayaneo: Add AYANEO 3 detachable controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Out-of-Bounds/NULL Pointer Dereference when `hdev->maxcollection == 0`
- [High] Use-After-Free due to Devres LIFO Violation on `hid_hw_stop`
- [Medium] Resource Lifecycle Stall in `eject_store` on hardware errors
- [Medium] Data Race allowing new commands to read a stale payload
--

--- Patch [1]: [PATCH] HID: ayaneo: Add AYANEO 3 detachable controller driver ---
commit fb4fc36a684a0e10b09f75f11cd5139de0102be2
Author: Matías Martínez <hello@matias.me>

HID: ayaneo: Add AYANEO 3 detachable controller driver

The AYANEO 3 handheld has a detachable controller with swappable
modules ("Magic Modules"). The controller exposes three USB HID
interfaces behind 1c4f:0002 (a generic SigmaMicro VID/PID, hence the
DMI gate): a gamepad, a keyboard for the extra buttons, and a vendor
interface accepting 65-byte commands.

> diff --git a/drivers/hid/hid-ayaneo.c b/drivers/hid/hid-ayaneo.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hid/hid-ayaneo.c
[ ... ]
> +static int aya3_cmd(struct aya3 *aya, u8 *resp)
> +{
> +	int attempt, ret;
> +
> +	lockdep_assert_held(&aya->lock);
> +
> +	for (attempt = 0; attempt < AYA3_CMD_ATTEMPTS; attempt++) {
> +		reinit_completion(&aya->resp_done);
> +		aya->resp_expect = aya->xfer[4];
> +		WRITE_ONCE(aya->resp_pending, true);
> +
> +		ret = aya3_send(aya);
> +		if (ret) {
> +			WRITE_ONCE(aya->resp_pending, false);
> +			return ret;
> +		}
> +
> +		if (wait_for_completion_timeout(&aya->resp_done,
> +						msecs_to_jiffies(AYA3_CMD_TIMEOUT_MS))) {
> +			if (resp)
> +				memcpy(resp, aya->resp, AYA3_RESP_SIZE);
> +			return 0;
> +		}
> +	}
> +	WRITE_ONCE(aya->resp_pending, false);
> +	return -ETIMEDOUT;
> +}
[ ... ]
> +static int aya3_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			  u8 *data, int size)
> +{
> +	struct aya3 *aya = hid_get_drvdata(hdev);
> +
> +	if (!READ_ONCE(aya->resp_pending) || size < AYA3_RESP_SIZE)
> +		return 0;
> +	if (data[AYA3_RESP_CMD] != aya->resp_expect)
> +		return 0;
> +
> +	memcpy(aya->resp, data, AYA3_RESP_SIZE);
> +	WRITE_ONCE(aya->resp_pending, false);
> +	complete(&aya->resp_done);
> +	return 0;
> +}

[Severity: Medium]
Is there a race condition here if a command times out just as the reply
arrives?

If aya3_cmd() times out, it clears aya->resp_pending and returns. However,
a concurrent aya3_raw_event() might have already read aya->resp_pending as
true. 

If a new command is issued immediately, could this delayed event copy stale
data and trigger complete() for the new command?

[ ... ]
> +static ssize_t eject_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
[ ... ]
> +	/*
> +	 * Wait for the firmware to report the eject as done. Userspace
> +	 * must then cut power through ayaneo-ec's controller_power for
> +	 * the module to be physically released.
> +	 */
> +	ret = -ETIMEDOUT;
> +	for (i = 0; i < 20; i++) {
> +		msleep(400);
> +		if (aya3_check(aya, resp))
> +			continue;
> +		if (!(resp[AYA3_RESP_EJECT_STATUS] & ~AYA3_EJECT_DONE_MASK)) {
> +			ret = 0;
> +			break;
> +		}
> +	}

[Severity: Medium]
Could this loop stall for up to 8 seconds if the hardware is abruptly
unplugged?

If the device is physically removed, aya3_check() will immediately return
a fatal error, such as -ENODEV.

Since this continue statement ignores the error unconditionally, won't it keep
sleeping for all 20 iterations even though the hardware is permanently gone?

[ ... ]
> +static int aya3_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct aya3 *aya;
> +	int ret;
> +
> +	/* The VID/PID is a generic SigmaMicro ID; bind on AYANEO 3 only */
> +	if (!dmi_check_system(aya3_dmi_table))
> +		return -ENODEV;
> +
> +	if (!hid_is_usb(hdev))
> +		return -ENODEV;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Bind only the vendor interface, not the gamepad/keyboard ones */
> +	if (hdev->collection->usage != (HID_UP_MSVENDOR | 0x0001))
> +		return -ENODEV;

[Severity: High]
Can this dereference a NULL or uninitialized pointer if the device presents a
report descriptor with zero collections?

A malformed USB HID device could pass hid_parse() but leave
hdev->maxcollection as 0.

Should this code check that hdev->maxcollection is greater than 0 before
accessing hdev->collection->usage?

[ ... ]
> +static void aya3_remove(struct hid_device *hdev)
> +{
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}

[Severity: High]
Does this manual hid_hw_stop() call violate the devres release order and
create a use-after-free window?

The driver registers the LED class device earlier in aya3_probe() using
the managed devm_led_classdev_multicolor_register().

Because devres actions run after the aya3_remove() callback finishes, calling
hid_hw_stop() here tears down the HID transport while the LED sysfs attributes
are still active.

If user space writes to the LED sysfs attribute during this window,
could aya3_led_set() attempt to send commands over the stopped transport?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824215041.79892-1-hello@matias.me?part=1