Re: [PATCH 06/10] HID: steam: Fully unregister controller when hidraw is opened

[sashiko-bot@kernel.org]

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

New issues:
- [High] Repeatedly opening and closing the hidraw node causes an unbounded kernel memory leak due to devm_kasprintf() allocations.
- [Medium] Unregistering and reregistering the controller on hidraw open/close causes connection state messages to spam the kernel log.

Pre-existing issues:
- [High] Error paths in steam_probe fail to clean up the global steam_devices list, leading to a use-after-free.
--

commit e81d4b5bb1894be8818d15e10c2e8f4c979081fe
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Fully unregister controller when hidraw is opened
    
To avoid conflicts between anything touching the hidraw and the driver we
had previously detached the evdev nodes when the hidraw is opened. However,
this isn't sufficient to avoid FEATURE reports from conflicting, so we
change to fully unregistering the controller internally, leaving only the
hidraw active until it's closed.

This also unifies the unregister and connect callbacks, as now the logic
between these two callbacks is identical.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 01b64194ec97b..546916da31f92 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -1144,38 +1143,41 @@ static int steam_register(struct steam_device *steam)
>  
>  static void steam_unregister(struct steam_device *steam)
>  {
> +	if (!steam->serial_no[0])
> +		return;
> +
> +	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> +			steam->serial_no);

[Severity: Medium]
Will repeatedly opening the hidraw node spam the kernel log?

Opening the hidraw node triggers this unregister path, printing a
"disconnected" message. When hidraw is closed, steam_register() is called and
logs a "connected" message.

Can a user-space daemon or application opening and closing the hidraw node
overwrite important system logs?

>  	steam_battery_unregister(steam);
>  	steam_sensors_unregister(steam);
>  	steam_input_unregister(steam);
> -	if (steam->serial_no[0]) {
> -		hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> -				steam->serial_no);
> -		mutex_lock(&steam_devices_lock);
> -		list_del_init(&steam->list);
> -		mutex_unlock(&steam_devices_lock);
> -		steam->serial_no[0] = 0;
> -	}
> +	mutex_lock(&steam_devices_lock);
> +	list_del_init(&steam->list);
> +	mutex_unlock(&steam_devices_lock);
> +	steam->serial_no[0] = 0;
>  }
>  
>  static void steam_work_connect_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							work_connect);
> +
>  	unsigned long flags;
>  	bool connected;
> +	bool opened;
>  	int ret;
>  
>  	spin_lock_irqsave(&steam->lock, flags);
> +	opened = steam->client_opened;
>  	connected = steam->connected;
>  	spin_unlock_irqrestore(&steam->lock, flags);
>  
> -	if (connected) {
> +	if (connected && !opened) {
>  		ret = steam_register(steam);

[Severity: High]
Does this cause a memory leak by repeatedly registering the battery?

When hidraw is opened, steam_unregister() is called, which sets
steam->serial_no[0] = 0.

When hidraw is closed, steam_register() sees the cleared serial number and
calls steam_battery_register():

drivers/hid/hid-steam.c:steam_register() {
    ...
	if (!steam->serial_no[0]) {
        ...
		if (steam->quirks & STEAM_QUIRK_WIRELESS)
			steam_battery_register(steam);
    ...
}

drivers/hid/hid-steam.c:steam_battery_register() {
    ...
	steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
			GFP_KERNEL, "steam-controller-%s-battery",
			steam->serial_no);
    ...
}

Because devm_ allocations are bound to the physical device's lifetime, and the
device is never unbound during this process, will repeatedly opening and
closing hidraw cause an unbounded memory leak?

> -		if (ret) {
> +		if (ret)
>  			hid_err(steam->hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -		}
>  	} else {
>  		steam_unregister(steam);
>  	}

[ ... ]

> @@ -1453,7 +1429,6 @@ static int steam_probe(struct hid_device *hdev,
>  	cancel_delayed_work_sync(&steam->mode_switch);
>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	cancel_work_sync(&steam->unregister_work);
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but do the error paths in steam_probe() fail
to clean up the global steam_devices list?

If steam_register() fails halfway (e.g., steam_input_register() fails), it
returns an error but does not remove the device from the steam_devices list
which it added it to earlier:

drivers/hid/hid-steam.c:steam_register() {
    ...
	if (!steam->serial_no[0]) {
        ...
		mutex_lock(&steam_devices_lock);
		if (list_empty(&steam->list))
			list_add(&steam->list, &steam_devices);
		mutex_unlock(&steam_devices_lock);
	}
    ...
}

When steam_probe() receives the error, it jumps to err_hw_close, bypassing
err_steam_unregister:

drivers/hid/hid-steam.c:steam_probe() {
    ...
	} else {
		/* A wired connection is always present */
		steam->connected = true;
		ret = steam_register(steam);
		if (ret) {
			hid_err(hdev,
				"%s:steam_register failed with error %d\n",
				__func__, ret);
			goto err_hw_close;
		}
	}
    ...
}

The steam object is subsequently freed by devres, leaving a dangling pointer
in the steam_devices list.

Can a local user trigger a use-after-free by writing to the lizard_mode
module parameter (which iterates over steam_devices)?

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