Re: [PATCH] HID: hyperx-headset: Add support for HyperX headset devices
From: sashiko-bot@kernel.org
Date: 2026-06-22 02:28:01
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Inverted teardown order in `hyperx_headset_remove` leads to Use-After-Free. - [High] Missing lock during power supply unregistration causes a data race and Use-After-Free. - [High] Missing work cancellation on `hid_hw_start` error path leads to Use-After-Free. - [Medium] Hardcoded power supply name causes sysfs collisions for multiple devices or interfaces. -- commit 435d0aab64fdcdfdde5a060312bb493c98996f6a Author: Sofia Schneider [off-list ref] HID: hyperx-headset: Add support for HyperX headset devices Introduce a HID driver for HyperX Cloud III Wireless headsets, supporting battery reporting and connection status.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-hyperx-headset.c b/drivers/hid/hid-hyperx-headset.c new file mode 100644 --- /dev/null +++ b/drivers/hid/hid-hyperx-headset.c
[ ... ]
+static const struct power_supply_desc hyperx_headset_battery_desc = {
+ .name = "hyperx_headset_battery",
+ .type = POWER_SUPPLY_TYPE_BATTERY,[Severity: Medium] Does this static power supply name cause sysfs collisions? If a user connects multiple identical headsets, or if the headset exposes multiple HID interfaces that bind to this driver, the second instance will fail to register with a -EEXIST error. Could this be dynamically allocated, perhaps using devm_kasprintf with hdev->uniq or a counter, to ensure each device gets a unique name?
+ .properties = hyperx_headset_battery_props, + .num_properties = ARRAY_SIZE(hyperx_headset_battery_props), + .get_property = hyperx_headset_battery_get_property, +};
[ ... ]
+static int hyperx_headset_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{[ ... ]
+ ret = hid_parse(hdev);
+ if (ret != 0) {
+ hid_err(hdev, "parse failed\n");
+ return ret;
+ }
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret != 0) {
+ hid_err(hdev, "hw_start failed\n");
+ return ret;
+ }[Severity: High] Can this error path leak pending work items? The hid_hw_start() call initializes the low-level driver, which can immediately start receiving hardware interrupts before completing the logical HID connection. If a packet arrives during this window, hyperx_headset_raw_event() can schedule drvdata->battery_work. If hid_hw_start() fails and returns an error, the function exits without calling cancel_work_sync(&drvdata->battery_work). The drvdata structure is then freed by devres, leaving a pending work item that will dereference freed memory when it executes. Should the error path ensure background work is cancelled before returning?
+ + schedule_delayed_work(&drvdata->poll_work, 0); + + return 0; +}
[ ... ]
+static void hyperx_headset_remove(struct hid_device *hdev)
+{
+ struct hyperx_headset_device *drvdata = hid_get_drvdata(hdev);
+
+ if (drvdata) {
+ cancel_delayed_work_sync(&drvdata->poll_work);
+ cancel_work_sync(&drvdata->battery_work);[Severity: High] Could this teardown sequence result in re-queued work? The driver calls cancel_work_sync() before calling hid_hw_stop(). This leaves a window where the hardware is still active and can generate interrupts after the work queues have been canceled. If a device state change event arrives during this window, hyperx_headset_raw_event() will call schedule_work(&drvdata->battery_work). When hyperx_headset_remove() completes, devres frees the drvdata structure. The incorrectly re-queued work item will execute later, dereferencing the freed drvdata memory. Should hid_hw_stop() be called before cancelling the work queues?
+
+ if (drvdata->battery) {
+ power_supply_unregister(drvdata->battery);
+ drvdata->battery = NULL;
+ }[Severity: High] Is it safe to access and clear drvdata->battery here without holding drvdata->lock? If an interrupt arrives concurrently, hyperx_headset_raw_event() will acquire the lock, observe a non-null drvdata->battery, and call power_supply_changed(drvdata->battery). Since power_supply_unregister() can concurrently drop the device reference and free the structure, power_supply_changed() might access freed memory. Should the pointer be cleared to null under the spinlock before unregistering, similar to how it is handled in hyperx_headset_battery_work()?
+ } + + hid_hw_stop(hdev); +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260622021744.145340-1-sofia@schn.dev?part=1