Re: [PATCH v2 1/6] HID: steam: Unregister battery on partial registration error

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Unregistering the battery in the error path prevents it from being re-registered on a subsequent retry of steam_register() because `steam->serial_no[0]` is not cleared.

Pre-existing issues:
- [High] The local variable `do_add` is conditionally initialized but unconditionally evaluated if `steam_register()` is called when `steam->serial_no[0]` is already set.
- [High] Teardown asymmetry in the `steam_probe` error path allows background work to register devices against a freed driver struct.
- [High] The unregister functions read RCU-protected pointers without writer-side serialization, leading to potential double-frees if executed concurrently.
--

--- Patch [1]: [PATCH v2 1/6] HID: steam: Unregister battery on partial registration error ---
commit 44ddde383b3d91088838e48fa3234971272a3bbc
Author: Vicki Pfau <[email protected]>

HID: steam: Unregister battery on partial registration error

This commit adds missing code to unregister the battery if later
registration steps fail. This prevents a lingering power_supply object from
being left over.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 6199f67f3c4cd..967f12c7a599a 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1199,6 +1199,7 @@ static int steam_register(struct steam_device *steam)
>  steam_register_sensors_fail:
>  	steam_input_unregister(steam);
>  steam_register_input_fail:
> +	steam_battery_unregister(steam);

[Severity: Medium]
Will this prevent the battery from being re-registered on a subsequent retry?

If steam_register() fails, the error path now unregisters the battery.
However, steam->serial_no[0] remains populated.

On a subsequent retry from steam_work_connect_cb(), the populated serial
number bypasses the initialization block, skipping steam_battery_register().

>  	return ret;
>  }

While reviewing this, I noticed a few pre-existing issues:

drivers/hid/hid-steam.c:steam_register() {
	bool do_add;
	...
	if (!steam->serial_no[0]) {
		...
		do_add = true;
	}
	...
	if (do_add) {

[Severity: High]
This is a pre-existing issue, but does this evaluate an uninitialized
variable?

If steam_register() is called when steam->serial_no[0] is already populated,
do_add is left uninitialized but is evaluated later in the function.


drivers/hid/hid-steam.c:steam_probe() {
	...
err_steam_unregister:
	if (steam->connected)
		steam_unregister(steam);
err_hw_close:
	hid_hw_close(hdev);
err_hw_stop:
	hid_hw_stop(hdev);
err_cancel_work:
	cancel_work_sync(&steam->work_connect);
	...
}

[Severity: High]
This is a pre-existing issue, but could this error path lead to a
use-after-free?

If a wireless device schedules work_connect and probe fails,
steam_unregister() is called before cancel_work_sync().

If work_connect hasn't initialized the device yet, steam_unregister()
returns without effect. Then cancel_work_sync() waits for work_connect to
finish, allowing it to register input devices against the steam driver
struct which devres will free after probe returns.


drivers/hid/hid-steam.c:steam_battery_unregister() {
	...
	rcu_read_lock();
	battery = rcu_dereference(steam->battery);
	rcu_read_unlock();

	if (!battery)
		return;
	RCU_INIT_POINTER(steam->battery, NULL);
	...
}

[Severity: High]
This is a pre-existing issue, but can this lead to a double-free if executed
concurrently?

The unregister functions read the RCU-protected pointers without writer-side
serialization.

If steam_probe() error path calls steam_unregister() concurrently with
work_connect() calling a failure path in steam_register(), both threads
might read the same non-NULL pointer and concurrently free the same
resources.

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