Re: [PATCH v4 04/11] HID: steam: Zero-initialize reply in serial lookup

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

Pre-existing issues:
- [High] Improper teardown ordering in `steam_remove()` allows works to be re-scheduled and execute after `steam_device` is freed.
- [High] Race condition in `steam_probe()` error path allows a background work item to register a device and corrupt global state after error unwinding.
--

--- Patch [4]: [PATCH v4 04/11] HID: steam: Zero-initialize reply in serial lookup ---
commit 3fb5af0927e3da21c09244521c52f446d7eb0ba2
Author: Vicki Pfau <[email protected]>

    HID: steam: Zero-initialize reply in serial lookup

    This patch eagerly zero-initializes the reply buffer in steam_get_serial()
    instead of just null-terminating it at the end. This prevents potential
    leaks of uninitialized memory in error logging paths.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 983d18d1de4fe..8bad79205b570 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -447,7 +447,7 @@ static int steam_get_serial(struct steam_device *steam)
>  	 */
>  	int ret = 0;
>  	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
> -	u8 reply[3 + STEAM_SERIAL_LEN + 1];
> +	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
>  
>  	mutex_lock(&steam->report_mutex);
>  	ret = steam_send_report(steam, cmd, sizeof(cmd));

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for works to
be rescheduled after they are cancelled in steam_remove()?

Looking at the code, it appears work items are cancelled before the hardware
events are stopped:

drivers/hid/hid-steam.c:steam_remove() {
    ...
	cancel_delayed_work_sync(&steam->mode_switch);
	cancel_work_sync(&steam->work_connect);
	cancel_work_sync(&steam->rumble_work);
	cancel_work_sync(&steam->unregister_work);
    ...
	hid_hw_close(hdev);
	hid_hw_stop(hdev);
}

If the hardware is still active, could incoming events trigger
steam_raw_event() and reschedule these works? 

Since steam_device is allocated via devm_kzalloc(), could this lead to a
use-after-free if the device is unplugged and a pending work later accesses
the freed memory?


[Severity: High]
This is a pre-existing issue, but I have a question about whether the error
handling in steam_probe() properly synchronizes with the background worker
thread.

If an error occurs during probe (for example, if steam_create_client_hid()
fails), the error path executes:

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);
    ...
}

If the hardware concurrently triggers steam_work_connect_cb(), could the
worker thread be executing steam_register()?

In this scenario, steam_unregister() might do nothing if steam->serial_no
isn't set yet. Then cancel_work_sync() would block until the worker finishes
registering the input device and adding it to the global steam_devices list.

Once steam_probe() returns and the driver core frees the steam_device memory,
could this leave dangling pointers in the global list and input subsystem?

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