Thread (2 messages) 2 messages, 2 authors, 27d ago

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help