Re: [PATCH v4 5/6] HID: steam: Clean up locking

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

New issues:
- [High] Missing `report_mutex` acquisition in `steam_register` causes concurrent HID report sequence corruption.

Pre-existing issues:
- [High] Use-After-Free of `steam->client_hdev` in `steam_raw_event` due to incorrect teardown sequence.
--

--- Patch [5]: [PATCH v4 5/6] HID: steam: Clean up locking ---
commit b7298af16f52c40e58c726cf4668c6523e3cddaf
Author: Vicki Pfau <[email protected]>
Date:   Fri Aug 7 23:23:36 2026 +0000

    HID: steam: Clean up locking
    
    This cleans up several issues with locking behavior, including RCU accesses
    not being guarded behind a lock.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index d95296b28f92b..95e252665351c 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -633,7 +634,6 @@ static int steam_exchange_report(struct steam_device *steam, u8 *cmd, int csize,
>  	unsigned int retries = 5;
>  	int ret;
>  
> -	guard(mutex)(&steam->report_mutex);
>  	do {
>  		ret = steam_send_report(steam, cmd, csize);
>  		if (ret < 0)

[ ... ]

> @@ -1380,13 +1376,16 @@ static int steam_register(struct steam_device *steam)
>  {
>  	int ret;
>  
> +	mutex_lock(&steam->registration_mutex);
>  	/*
>  	 * This function can be called several times in a row with the
>  	 * wireless adaptor, without steam_unregister() between them, because
>  	 * another client send a get_connection_status command, for example.
>  	 */
> -	if (steam->registered)
> +	if (steam->registered) {
> +		mutex_unlock(&steam->registration_mutex);
>  		return 0;
> +	}
>  
>  	/*
>  	 * Unlikely, but getting the serial could fail, and it is not so

[Severity: High]
Does this leave the HID report sequence unprotected during device registration?

Because the patch removes the report_mutex from steam_exchange_report() and
steam_set_lizard_mode(), their callers are now expected to hold it. However,
steam_register() doesn't acquire report_mutex before it continues execution
and calls steam_get_serial() (which calls steam_exchange_report()) and
steam_set_lizard_mode().

For example, a wireless connection immediately after USB enumeration triggers
steam_raw_event() -> steam_do_connect_event(), which queues
steam_work_connect_cb to call steam_register(). At the same time, the
original steam_probe() thread continues and calls steam_get_conn_status(). 

Since steam_register() lacks the report_mutex, its HID transmissions could 
interleave with the internally-locked steam_get_conn_status() operations.
Can this lead to corrupted replies or matching a reply to the wrong request?

[ ... ]

> @@ -2470,6 +2466,7 @@ static int steam_raw_event(struct hid_device *hdev,
>  	if (!steam)
>  		return 0;
>  
> +	guard(spinlock_irqsave)(&steam->lock);
>  	if (steam->client_opened)
>  		hid_input_report(steam->client_hdev, report->type, data, size, 0);
>  

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free of
steam->client_hdev if an event arrives during teardown?

In steam_remove(), hid_destroy_device(steam->client_hdev) is called to
destroy the child HID device, but hid_hw_stop(hdev) is not called until the
very end of the function. This means hardware interrupts can still deliver
raw events. 

If steam_raw_event() triggers after the child device is destroyed, but
before steam->client_opened is explicitly cleared under the lock in
steam_remove(), it will dereference the freed steam->client_hdev pointer.

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