Re: [PATCH v7 6/8] HID: steelseries: Manage battery lifetime with refcounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent execution of `steelseries_arctis_raw_event()` and explicit battery unregistration in `steelseries_arctis_remove()` introduces a Use-After-Free and NULL pointer dereference.
- [Medium] The power supply name string is allocated with `devm_kasprintf` but is embedded inside a manually reference-counted object `sd`, leading to a lifecycle mismatch.
--

--- Patch [6]: [PATCH v7 6/8] HID: steelseries: Manage battery lifetime with refcounting ---
commit fd37e70cee8d0953a9d3de05f0149d2fc76d513a
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Manage battery lifetime with refcounting

This commit changes the steelseries_device structure to use a kref for
lifetime management instead of devm. It explicitly registers and
unregisters the battery power supply and nullifies the battery pointer
during removal.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index b54f20f081a26..e534aa44e70a0 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c

[ ... ]

> @@ -277,9 +288,10 @@ static int steelseries_battery_register(struct steelseries_device *sd)
>  	sd->battery_desc.num_properties = ARRAY_SIZE(steelseries_battery_props);
>  	sd->battery_desc.get_property = steelseries_battery_get_property;
>  	sd->battery_desc.use_for_apm = 0;
> -	n = atomic_inc_return(&battery_no) - 1;
>  	sd->battery_desc.name = devm_kasprintf(&sd->hdev->dev, GFP_KERNEL,
> -						"steelseries_headset_battery_%ld", n);
> +					       "steelseries_headset_battery_%s",
> +					       sd->hdev->uniq[0] ? sd->hdev->uniq :
> +					       dev_name(&sd->hdev->dev));

[Severity: Medium]
Since the steelseries_device struct sd is now refcounted and can outlive
the device, will using devm_kasprintf() for sd->battery_desc.name leave a
dangling pointer inside sd when the device unbinds and devres frees the
string?

[ ... ]

> @@ -387,13 +405,20 @@ static void steelseries_arctis_remove(struct hid_device *hdev)
>  	if (interface_num == sd->info->sync_interface) {
>  		spin_lock_irqsave(&sd->lock, flags);
>  		sd->removed = true;
> +		battery = sd->battery;
> +		sd->battery = NULL;
>  		spin_unlock_irqrestore(&sd->lock, flags);
>  
>  		cancel_delayed_work_sync(&sd->status_work);
> +
> +		if (battery)
> +			power_supply_unregister(battery);
>  	}
>  
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

[Severity: High]
Can an incoming HID event trigger steelseries_arctis_raw_event() concurrently
with this teardown, leading to a NULL pointer dereference or use-after-free?

Since hid_hw_stop() hasn't been called yet, hardware events can still fire.
If steelseries_arctis_raw_event() locklessly checks sd->battery and then
calls power_supply_changed(sd->battery), could remove() nullify and free the
battery in between the check and the use?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803191553.66368-1-srimanachanta@gmail.com?part=6