Re: [PATCH] HID: hyperx-headset: Add support for HyperX headset devices

[sashiko-bot@kernel.org]

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 <sofia@schn.dev>

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.

> 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