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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `drvdata` structure is freed before the battery power supply is unregistered, leading to a Use-After-Free vulnerability if userspace reads battery properties during device removal.
- [Medium] The fallback logic for non-Pulsar devices during battery initialization is bypassed on command timeout.
- [Low] Syntax error in the `model` buffer size will prevent compilation.
--

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

    HID: pulsar: add driver for Pulsar gaming mice

This commit introduces a new HID driver for Pulsar wireless gaming mice
and related devices. It exposes battery level, voltage, and charging
status through the power supply framework.

> diff --git a/drivers/hid/hid-pulsar.c b/drivers/hid/hid-pulsar.c
> new file mode 100644
> index 0000000000000..a6ff6e436d407
> --- /dev/null
> +++ b/drivers/hid/hid-pulsar.c
> @@ -0,0 +1,763 @@
[ ... ]
> +struct pulsar_battery {
> +	struct power_supply *ps;
> +	struct power_supply_desc desc;
> +	char name[48];
> +	char model[MAX(32, sizeof((struct hid_device){}).name)];

[Severity: Low]
Does this compile? It looks like the sizeof operator is applied to the 
compound literal itself, which yields a size_t value, making the subsequent 
.name access a syntax error.

[ ... ]
> +static void pulsar_init_work(struct work_struct *work)
> +{
> +	struct pulsar_data *drvdata;
> +	struct hid_device *hdev;
> +	struct power_supply_config psy_cfg;
> +	int ret;
> +	u8 data[DEV_INFO_LEN];
> +
> +	drvdata = container_of(work, struct pulsar_data, init_work.work);
> +	hdev = drvdata->hdev;
> +
> +	ret = read_device_info(drvdata, data);
> +	if (ret == -ETIMEDOUT) {
> +		if (drvdata->init_retries--) {
> +			hid_dbg(hdev,
> +				"device info read timed out, retrying (%u left)\n",
> +				drvdata->init_retries);
> +			schedule_delayed_work(&drvdata->init_work,
> +					      msecs_to_jiffies
> +					      (INIT_DELAY_MSEC));
> +			return;
> +		}
> +		hid_err(hdev, "device info read timed out, giving up\n");
> +		return;

[Severity: Medium]
If a non-Pulsar device ignores the CMD_INFO request and times out, will it
skip the fallback logic?

The fallback logic for devices that fail to read info is located in the 
if (ret < 0) block below, which safely defaults to assigning hdev->name. 
If the command times out instead, it returns directly here, preventing 
the device from registering a battery at all.

> +	}
> +	if (ret < 0) {
[ ... ]
> +register_battery:
> +	init_power_supply_desc(drvdata);
> +
> +	psy_cfg = (struct power_supply_config) {.drv_data = drvdata };
> +	drvdata->battery.ps =
> +	    devm_power_supply_register(&hdev->dev, &drvdata->battery.desc,
> +				       &psy_cfg);

[Severity: Critical]
Can this cause a use-after-free during device removal?

Because devm_power_supply_register() is called asynchronously from a workqueue
rather than directly during the probe sequence, the devres allocation attaches
to the main device devres group instead of the HID probe devres group.

During unbind, the HID core will release the probe devres group and free
drvdata. However, the power supply remains registered until the main device
devres group is released later. 

If userspace reads a sysfs property during this window, battery_get_property()
could dereference the freed drvdata.

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