Re: [PATCH] HID: pulsar: add driver for Pulsar gaming mice

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect teardown order allows `power_uevent_work` to be re-queued after cancellation, causing Use-After-Free.
- [High] Manual destruction of `lock_cmd` mutex races with devres power supply unregistration, causing Use-After-Destroy.
--

--- Patch [1]: [PATCH] HID: pulsar: add driver for Pulsar gaming mice ---
commit 312ad5774c1622addffac4566b25aa23172cdfd6
Author: Nikolas Koesling <[email protected]>

HID: pulsar: add driver for Pulsar gaming mice

Add a HID driver for Pulsar wireless gaming mice (X2 V2, X2H, X2A,
Xlite V3). The driver exposes battery level, voltage, and charging
status through the power supply framework. It supports wired, 1kHz,
and 4kHz wireless dongle connections.

> diff --git a/drivers/hid/hid-pulsar.c b/drivers/hid/hid-pulsar.c
> new file mode 100644
> index 0000000000000..2720b9f18b08d
> --- /dev/null
> +++ b/drivers/hid/hid-pulsar.c
[ ... ]
> +static int pulsar_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
[ ... ]
> +	schedule_delayed_work(&drvdata->init_work, 0);
> +
> +	return 0;
> +
> +err_open:
> +	cancel_work_sync(&drvdata->power_uevent_work);
> +	hid_hw_stop(hdev);

[Severity: High]
Does this error path cancel the work while the hardware is still active?

If an incoming HID report triggers pulsar_raw_event() after this
cancel_work_sync() but before hid_hw_stop(), it will unconditionally queue
power_uevent_work again:

pulsar_raw_event() {
    ...
    if (data[6] == EVENT_PWR && data[USB_PAYLOAD_LEN - 1] == EVENT_PWR_CHK) {
        schedule_work(&drvdata->power_uevent_work);
    ...
}

The probe function would then return, and devres would automatically free
drvdata. When the workqueue subsequently executes the queued work, could
this lead to a use-after-free when accessing drvdata in
power_uevent_work_handler()?

> +	return ret;
> +}
> +
> +static void pulsar_remove(struct hid_device *hdev)
> +{
> +	struct pulsar_data *drvdata;
> +
> +	drvdata = hid_get_drvdata(hdev);
> +	if (!drvdata) {
> +		hid_hw_stop(hdev);
> +		return;
> +	}
> +
> +	atomic_set(&drvdata->stopping, 1);
> +	cancel_delayed_work_sync(&drvdata->init_work);
> +	cancel_work_sync(&drvdata->power_uevent_work);

[Severity: High]
Similar to the probe error path, is it safe to cancel work here before
calling hid_hw_stop(hdev)?

Because the underlying hardware transport (and interrupts) remains active
until hid_hw_stop() is called, an incoming HID report could queue
power_uevent_work again after cancellation, leading to a use-after-free
when devres cleans up drvdata.

> +
> +	/* wait for active device i/o (exec_cmd) */
> +	mutex_lock(&drvdata->lock_cmd);
> +	hid_hw_close(hdev);
> +	mutex_unlock(&drvdata->lock_cmd);
> +
> +	hid_hw_stop(hdev);
> +	mutex_destroy(&drvdata->lock_cmd);

[Severity: High]
Is it safe to manually destroy this mutex here before devres unregisters
the power supply?

Because the power supply is registered via devm_power_supply_register(),
devres does not unregister it until after pulsar_remove() completely
finishes.

During this window, a concurrent sysfs read can invoke:
battery_get_property() -> read_power() -> exec_cmd()

While exec_cmd() has an atomic_read(&drvdata->stopping) check, a sysfs
thread could pass that check just before stopping is set to 1, and then get
scheduled out.

When that thread resumes, could it attempt to lock the destroyed mutex,
resulting in a use-after-destroy?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1