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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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;
+}
[ ... ]
quoted hunk ↗ jump to hunk
+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?
[ ... ]
quoted hunk ↗ jump to hunk
+static ssize_t eject_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ /*
+ * 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?
[ ... ]
quoted hunk ↗ jump to hunk
+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?
[ ... ]
quoted hunk ↗ jump to hunk
+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