Re: [PATCH v3 07/10] HID: steam: Rearrange teardown sequences

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Moving hid_hw_start before initialization creates a window for Use-Before-Initialization
- [Critical] Probe error path skips cancellation of work_connect and mode_switch

Pre-existing issues:
- [Critical] Hardware interrupts can re-queue work_connect after it is cancelled during teardown
- [Critical] Failure inside steam_register leaves dangling pointers in a global list
- [Critical] Incoming reports can access a freed client_hdev due to lockless state checking
--

--- Patch [7]: [PATCH v3 07/10] HID: steam: Rearrange teardown sequences ---
commit 9242f209b3856143cba9d679406bd9c9784d93c2
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Rearrange teardown sequences

This patch fixes a narrow window during the teardown sequences in probe
and remove where callbacks could still be scheduled during cleanup. It
reorders the teardown steps to prevent dangling pointers to the freed
steam struct.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 0771ad4f5f2c2..9dc77cd76edb0 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1353,6 +1354,14 @@ static int steam_probe(struct hid_device *hdev,
>  	if (!steam)
>  		return -ENOMEM;
>  
> +	/*
> +	 * With the real steam controller interface, do not connect hidraw.
> +	 * Instead, create the client_hid and connect that.
> +	 */
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;
> +
>  	steam->hdev = hdev;
>  	hid_set_drvdata(hdev, steam);
>  	spin_lock_init(&steam->lock);

[Severity: Critical]
Does calling hid_hw_start() here enable hardware interrupts before the
steam struct is fully initialized? 

If an unprompted wireless connect event arrives immediately after
hid_set_drvdata(), could steam_raw_event() -> steam_do_connect_event()
attempt to acquire the uninitialized steam->lock and pass a zero-initialized
steam->work_connect to the workqueue?

[ ... ]

> @@ -1398,14 +1391,22 @@ static int steam_probe(struct hid_device *hdev,
>  			hid_err(hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -			goto err_hw_close;
> +			goto err_hw_stop;
>  		}
>  	}

[Severity: Critical]
If steam_register() fails and jumps directly to err_hw_stop, does this bypass
the cancellation of work_connect and mode_switch at the err_steam_unregister
label? 

Since hid_hw_start() is now called earlier, could an incoming interrupt have
already scheduled these works, resulting in a use-after-free when the steam
struct is freed?

[Severity: Critical]
This is a pre-existing issue, but if steam_register() fails here:

drivers/hid/hid-steam.c:steam_register() {
    ...
    mutex_lock(&steam_devices_lock);
    if (list_empty(&steam->list))
        list_add(&steam->list, &steam_devices);
    mutex_unlock(&steam_devices_lock);
    ...
    if (!client_opened) {
        ret = steam_input_register(steam);
        if (ret != 0)
            goto steam_register_input_fail;
        ...
}

Since the error path jumps to err_hw_stop, bypassing steam_unregister(),
does this leave a dangling pointer to the freed steam struct in the global
steam_devices list?

[ ... ]

> @@ -1436,25 +1439,27 @@ static int steam_probe(struct hid_device *hdev,
>  static void steam_remove(struct hid_device *hdev)
>  {
>  	struct steam_device *steam = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
>  	if (!steam || hdev->group == HID_GROUP_STEAM) {
>  		hid_hw_stop(hdev);
>  		return;
>  	}
>  
> +	hid_hw_close(hdev);
>  	hid_destroy_device(steam->client_hdev);
> -	cancel_delayed_work_sync(&steam->mode_switch);
> -	cancel_work_sync(&steam->work_connect);
> -	cancel_work_sync(&steam->rumble_work);
> -	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	steam->client_hdev = NULL;
> +	spin_lock_irqsave(&steam->lock, flags);
>  	steam->client_opened = 0;
> +	spin_unlock_irqrestore(&steam->lock, flags);

[Severity: Critical]
This isn't a bug introduced by this patch, but calling
hid_destroy_device(steam->client_hdev) before clearing steam->client_opened
seems risky.

Since steam_raw_event() checks client_opened without holding locks or memory
barriers:

drivers/hid/hid-steam.c:steam_raw_event() {
    ...
    if (steam->client_opened)
        hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
                         data, size, 0);
    ...
}

Could a concurrent interrupt read a stale non-zero value for client_opened
and access the already-freed client_hdev?

> +	cancel_work_sync(&steam->work_connect);
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);
> +	cancel_work_sync(&steam->rumble_work);
> +	cancel_delayed_work_sync(&steam->mode_switch);
> +	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> +	hid_hw_stop(hdev);
>  }

[Severity: Critical]
This is a pre-existing issue, but does calling
cancel_work_sync(&steam->work_connect) while hardware interrupts are still
active leave a race window?

If another user has the device open, hid_hw_close() doesn't fully stop the
transport interrupts until hid_hw_stop() is called. 

Could an incoming ID_CONTROLLER_WIRELESS report trigger
steam_do_connect_event() and unconditionally re-queue work_connect after it
was cancelled, leading to a use-after-free when the steam struct is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710020048.2337655-1-vi@endrift.com?part=7