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

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [High] Concurrent execution of `steelseries_arctis_raw_event()` and expli=
cit battery unregistration in `steelseries_arctis_remove()` introduces a Us=
e-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 wit=
h refcounting ---
commit fd37e70cee8d0953a9d3de05f0149d2fc76d513a
Author: Sriman Achanta <[email protected]>

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-steel=
series-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 steel=
series_device *sd)
>  	sd->battery_desc.num_properties =3D ARRAY_SIZE(steelseries_battery_prop=
s);
>  	sd->battery_desc.get_property =3D steelseries_battery_get_property;
>  	sd->battery_desc.use_for_apm =3D 0;
> -	n =3D atomic_inc_return(&battery_no) - 1;
>  	sd->battery_desc.name =3D 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_de=
vice *hdev)
>  	if (interface_num =3D=3D sd->info->sync_interface) {
>  		spin_lock_irqsave(&sd->lock, flags);
>  		sd->removed =3D true;
> +		battery =3D sd->battery;
> +		sd->battery =3D NULL;
>  		spin_unlock_irqrestore(&sd->lock, flags);
> =20
>  		cancel_delayed_work_sync(&sd->status_work);
> +
> +		if (battery)
> +			power_supply_unregister(battery);
>  	}
> =20
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

[Severity: High]
Can an incoming HID event trigger steelseries_arctis_raw_event() concurrent=
ly
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803191553.6636=
[email protected]?part=3D6