[STABLE v5.15-v5.10 1/6] HID: hid-steam: Fix cleanup in probe()

[STABLE v5.15-v5.10 1/6] HID: hid-steam: Fix cleanup in probe()

[Lee Jones]

From: Dan Carpenter <dan.carpenter@linaro.org>

[ Upstream commit a9f1da09c69f13ef471db8b22107a28042d230ca ]

There are a number of issues in this code.  First of all if
steam_create_client_hid() fails then it leads to an error pointer
dereference when we call hid_destroy_device(steam->client_hdev).

Also there are a number of leaks.  hid_hw_stop() is not called if
hid_hw_open() fails for example.  And it doesn't call steam_unregister()
or hid_hw_close().

Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Vicki Pfau <vi@endrift.com>
Link: https://lore.kernel.org/r/1fd87904-dabf-4879-bb89-72d13ebfc91e@moroto.mountain
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
[Lee: Move client_hdev creation after steam_register() and omit
      mode_switch and rumble_work cancellation as 691ead124a0c and
      e133dc99fc07 are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 43 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index fc616db4231b..8832f7987006 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -751,31 +751,20 @@ static int steam_probe(struct hid_device *hdev,
 	INIT_WORK(&steam->work_connect, steam_work_connect_cb);
 	INIT_LIST_HEAD(&steam->list);
 
-	steam->client_hdev = steam_create_client_hid(hdev);
-	if (IS_ERR(steam->client_hdev)) {
-		ret = PTR_ERR(steam->client_hdev);
-		goto client_hdev_fail;
-	}
-	steam->client_hdev->driver_data = steam;
-
 	/*
 	 * 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)
-		goto hid_hw_start_fail;
-
-	ret = hid_add_device(steam->client_hdev);
-	if (ret)
-		goto client_hdev_add_fail;
+		goto err_cancel_work;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev,
 			"%s:hid_hw_open\n",
 			__func__);
-		goto hid_hw_open_fail;
+		goto err_hw_stop;
 	}
 
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
@@ -791,19 +780,33 @@ static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto input_register_fail;
+			goto err_hw_close;
 		}
 	}
 
+	steam->client_hdev = steam_create_client_hid(hdev);
+	if (IS_ERR(steam->client_hdev)) {
+		ret = PTR_ERR(steam->client_hdev);
+		goto err_stream_unregister;
+	}
+	steam->client_hdev->driver_data = steam;
+
+	ret = hid_add_device(steam->client_hdev);
+	if (ret)
+		goto err_destroy;
+
 	return 0;
 
-input_register_fail:
-hid_hw_open_fail:
-client_hdev_add_fail:
-	hid_hw_stop(hdev);
-hid_hw_start_fail:
+err_destroy:
 	hid_destroy_device(steam->client_hdev);
-client_hdev_fail:
+err_stream_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);
 steam_alloc_fail:
 	hid_err(hdev, "%s: failed with error %d\n",
-- 
2.56.0.385.gd3acb90ef8-goog

[STABLE v5.15-v5.10 2/6] HID: hid-steam: Make client_opened a counter

[Lee Jones]

From: Vicki Pfau <vi@endrift.com>

[ Upstream commit 555b818adb97eca70210a49ba3f1d27882dde092 ]

The client_opened variable was used to track if the hidraw was opened by any
clients to silence keyboard/mouse events while opened. However, there was no
counting of how many clients were opened, so opening two at the same time and
then closing one would fool the driver into thinking it had no remaining opened
clients.

Signed-off-by: Vicki Pfau <vi@endrift.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Adapt steam_client_ll_open() and steam_client_ll_close() as
      691ead124a0c and e1147961b214 are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 8832f7987006..159b55dff799 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -114,7 +114,7 @@ struct steam_device {
 	spinlock_t lock;
 	struct hid_device *hdev, *client_hdev;
 	struct mutex mutex;
-	bool client_opened;
+	unsigned long client_opened;
 	struct input_dev __rcu *input;
 	unsigned long quirks;
 	struct work_struct work_connect;
@@ -509,7 +509,7 @@ static void steam_battery_unregister(struct steam_device *steam)
 static int steam_register(struct steam_device *steam)
 {
 	int ret;
-	bool client_opened;
+	unsigned long client_opened;
 
 	/*
 	 * This function can be called several times in a row with the
@@ -634,7 +634,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
 	struct steam_device *steam = hdev->driver_data;
 
 	mutex_lock(&steam->mutex);
-	steam->client_opened = true;
+	steam->client_opened++;
 	mutex_unlock(&steam->mutex);
 
 	steam_input_unregister(steam);
@@ -654,12 +654,12 @@ static void steam_client_ll_close(struct hid_device *hdev)
 	spin_unlock_irqrestore(&steam->lock, flags);
 
 	mutex_lock(&steam->mutex);
-	steam->client_opened = false;
-	if (connected)
+	steam->client_opened--;
+	if (connected && !steam->client_opened)
 		steam_set_lizard_mode(steam, lizard_mode);
 	mutex_unlock(&steam->mutex);
 
-	if (connected)
+	if (connected && !steam->client_opened)
 		steam_input_register(steam);
 }
 
@@ -824,7 +824,7 @@ static void steam_remove(struct hid_device *hdev)
 	}
 
 	hid_destroy_device(steam->client_hdev);
-	steam->client_opened = false;
+	steam->client_opened = 0;
 	cancel_work_sync(&steam->work_connect);
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver disconnected");
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [STABLE v5.15-v5.10 2/6] HID: hid-steam: Make client_opened a counter

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hid: hid-steam: Redundant manual refcounting in open/close callbacks
--

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

HID: hid-steam: Make client_opened a counter

This patch changes the client_opened flag from a boolean to a counter. The
intent is to track how many clients have opened the hidraw node so that
concurrent opens and closes do not prematurely re-enable lizard mode.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 379e7765a93e9..78104e1246e82 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -624,7 +624,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
>  	struct steam_device *steam = hdev->driver_data;
>  
>  	mutex_lock(&steam->mutex);
> -	steam->client_opened = true;
> +	steam->client_opened++;

[Severity: Low]
This isn't a bug, but is this manual refcounting redundant?

The commit message indicates this change is to handle multiple clients opening
the device at the same time. However, the HID core already natively refcounts
and serializes the low-level open and close callbacks.

Looking at hid_hw_open() in drivers/hid/hid-core.c:

	if (!hdev->ll_open_count++) {
		ret = hdev->ll_driver->open(hdev);
		...
	}

Since the core only calls the low-level open callback on the 0 to 1 transition
and the close callback on the 1 to 0 transition, concurrent opens are handled
before reaching steam_client_ll_open(). Does this mean the manual counter
behaves identically to the original boolean flag and won't actually track
multiple clients at this level?

>  	mutex_unlock(&steam->mutex);
>  
>  	steam_input_unregister(steam);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091215.529531-1-lee@kernel.org?part=2

[STABLE v5.15-v5.10 3/6] HID: steam: Fully unregister controller when hidraw is opened

[Lee Jones]

From: Vicki Pfau <vi@endrift.com>

[ Upstream commit cd33a91d37eb4d7c6ce56aa7f4688066309808eb ]

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.

Signed-off-by: Vicki Pfau <vi@endrift.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Convert steam_client_ll_open() and steam_client_ll_close() to
      schedule work_connect directly and omit sensors, rumble_work,
      mode_switch and unregister_work changes as e133dc99fc07,
      e1147961b214 and 1d4549fd1115 are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 78 ++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 159b55dff799..a856276b6c2c 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -380,6 +380,7 @@ static int steam_battery_register(struct steam_device *steam)
 			&steam->battery_desc, &battery_cfg);
 	if (IS_ERR(battery)) {
 		ret = PTR_ERR(battery);
+		devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
 		hid_err(steam->hdev,
 				"%s:power_supply_register failed with error %d\n",
 				__func__, ret);
@@ -504,12 +505,15 @@ static void steam_battery_unregister(struct steam_device *steam)
 	RCU_INIT_POINTER(steam->battery, NULL);
 	synchronize_rcu();
 	power_supply_unregister(battery);
+	devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
 }
 
 static int steam_register(struct steam_device *steam)
 {
 	int ret;
 	unsigned long client_opened;
+	unsigned long flags;
+	bool do_add;
 
 	/*
 	 * This function can be called several times in a row with the
@@ -535,38 +539,47 @@ static int steam_register(struct steam_device *steam)
 		if (steam->quirks & STEAM_QUIRK_WIRELESS)
 			steam_battery_register(steam);
 
-		mutex_lock(&steam_devices_lock);
-		if (list_empty(&steam->list))
-			list_add(&steam->list, &steam_devices);
-		mutex_unlock(&steam_devices_lock);
+		do_add = true;
 	}
 
-	mutex_lock(&steam->mutex);
+	spin_lock_irqsave(&steam->lock, flags);
 	client_opened = steam->client_opened;
-	if (!client_opened)
-		steam_set_lizard_mode(steam, lizard_mode);
-	mutex_unlock(&steam->mutex);
+	spin_unlock_irqrestore(&steam->lock, flags);
 
-	if (!client_opened)
+	if (!client_opened) {
+		mutex_lock(&steam->mutex);
+		steam_set_lizard_mode(steam, lizard_mode);
+		mutex_unlock(&steam->mutex);
 		ret = steam_input_register(steam);
-	else
-		ret = 0;
+		if (ret != 0)
+			goto steam_register_input_fail;
+	}
 
+	if (do_add) {
+		mutex_lock(&steam_devices_lock);
+		if (list_empty(&steam->list))
+			list_add(&steam->list, &steam_devices);
+		mutex_unlock(&steam_devices_lock);
+	}
+	return 0;
+
+steam_register_input_fail:
 	return ret;
 }
 
 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);
 	steam_battery_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)
@@ -575,19 +588,20 @@ static void steam_work_connect_cb(struct work_struct *work)
 							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);
-		if (ret) {
+		if (ret)
 			hid_err(steam->hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-		}
 	} else {
 		steam_unregister(steam);
 	}
@@ -632,12 +646,13 @@ static void steam_client_ll_stop(struct hid_device *hdev)
 static int steam_client_ll_open(struct hid_device *hdev)
 {
 	struct steam_device *steam = hdev->driver_data;
+	unsigned long flags;
 
-	mutex_lock(&steam->mutex);
+	spin_lock_irqsave(&steam->lock, flags);
 	steam->client_opened++;
-	mutex_unlock(&steam->mutex);
+	spin_unlock_irqrestore(&steam->lock, flags);
 
-	steam_input_unregister(steam);
+	schedule_work(&steam->work_connect);
 
 	return 0;
 }
@@ -645,22 +660,13 @@ static int steam_client_ll_open(struct hid_device *hdev)
 static void steam_client_ll_close(struct hid_device *hdev)
 {
 	struct steam_device *steam = hdev->driver_data;
-
 	unsigned long flags;
-	bool connected;
 
 	spin_lock_irqsave(&steam->lock, flags);
-	connected = steam->connected;
-	spin_unlock_irqrestore(&steam->lock, flags);
-
-	mutex_lock(&steam->mutex);
 	steam->client_opened--;
-	if (connected && !steam->client_opened)
-		steam_set_lizard_mode(steam, lizard_mode);
-	mutex_unlock(&steam->mutex);
+	spin_unlock_irqrestore(&steam->lock, flags);
 
-	if (connected && !steam->client_opened)
-		steam_input_register(steam);
+	schedule_work(&steam->work_connect);
 }
 
 static int steam_client_ll_raw_request(struct hid_device *hdev,
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [STABLE v5.15-v5.10 3/6] HID: steam: Fully unregister controller when hidraw is opened

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: steam: Use-after-free of battery name in class iterators
--

commit ac4de628ad44ddc16c1b1fd80453897f658181f6
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, this
commit changes the driver to fully unregister the controller internally when
hidraw is opened. It also unifies the unregister and connect callbacks.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 78104e1246e82..cf1a3da36478c 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -494,12 +495,15 @@ static void steam_battery_unregister(struct steam_device *steam)
>  	RCU_INIT_POINTER(steam->battery, NULL);
>  	synchronize_rcu();
>  	power_supply_unregister(battery);
> +	devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
>  }

[Severity: High]
Can this introduce a use-after-free for the battery name string?

steam_battery_unregister() calls power_supply_unregister(battery), which
eventually delegates to device_del() and klist_del().

klist_del() drops the hierarchy reference asynchronously and does not block
waiting for active class iterators to finish.

If a concurrent caller to power_supply_get_by_name() iterates over the
power_supply_class, it will call power_supply_match_device_by_name() which
executes strcmp(psy->desc->name, name). 

Since devm_kfree() explicitly frees steam->battery_desc.name immediately
after unregistering, couldn't this dereference the freed name string and
cause a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091215.529531-1-lee@kernel.org?part=3

[STABLE v5.15-v5.10 4/6] HID: steam: Rearrange teardown sequence

[Lee Jones]

From: Vicki Pfau <vi@endrift.com>

[ Upstream commit ddce9641ce924ed91ec8304577e463823a29ffaf ]

This fixes a narrow window during the teardown sequence where callbacks
could still be scheduled during cleanup that would then have a dangling
pointer to the now-freed steam struct.

This also puts work canceling for rumble and mode switch in
steam_unregister, as that shouldn't persist while the client hdev is open.

Signed-off-by: Vicki Pfau <vi@endrift.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Omit rumble_work, coalesce_rumble_work and mode_switch
      cancellation as e133dc99fc07 and 1d4549fd1115 are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index a856276b6c2c..628e4ef72b81 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -829,15 +829,16 @@ static void steam_remove(struct hid_device *hdev)
 		return;
 	}
 
+	hid_hw_close(hdev);
 	hid_destroy_device(steam->client_hdev);
+	steam->client_hdev = NULL;
 	steam->client_opened = 0;
 	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);
+	hid_hw_stop(hdev);
 }
 
 static void steam_do_connect_event(struct steam_device *steam, bool connected)
-- 
2.56.0.385.gd3acb90ef8-goog

[STABLE v5.15-v5.10 5/6] HID: steam: Refactor registration

[Lee Jones]

From: Vicki Pfau <vi@endrift.com>

[ Upstream commit 6f9b740c31ba7e061b2ebb330d9a3d63f1358f29 ]

This refactors and simplifies the registration/unregistration flow. Since
we now only perform registration when the client isn't opened anymore, the
logic for handling that can be removed, and the rest of the function
streamlined.

We also remove the previous assumption that we have a serial number to show
we're registered, replacing it with a single purpose boolean.

In a previous refactor the code for unregistering a battery if later
registration steps failed was accidentally left out. As a result, a
lingering power_supply object could get left over after the steam object
was torn down. This is also fixed here.

Signed-off-by: Vicki Pfau <vi@endrift.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Omit sensors, steam_get_attributes(), rumble_work,
      coalesce_rumble_work and mode_switch changes as e133dc99fc07,
      a0fbd9437d7d and 1d4549fd1115 are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 70 ++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 628e4ef72b81..544d45f3fc6b 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -119,6 +119,7 @@ struct steam_device {
 	unsigned long quirks;
 	struct work_struct work_connect;
 	bool connected;
+	bool registered;
 	char serial_no[STEAM_SERIAL_LEN + 1];
 	struct power_supply_desc battery_desc;
 	struct power_supply __rcu *battery;
@@ -511,9 +512,6 @@ static void steam_battery_unregister(struct steam_device *steam)
 static int steam_register(struct steam_device *steam)
 {
 	int ret;
-	unsigned long client_opened;
-	unsigned long flags;
-	bool do_add;
 
 	/*
 	 * This function can be called several times in a row with the
@@ -521,59 +519,53 @@ static int steam_register(struct steam_device *steam)
 	 * another client send a get_connection_status command, for example.
 	 * The battery and serial number are set just once per device.
 	 */
-	if (!steam->serial_no[0]) {
-		/*
-		 * Unlikely, but getting the serial could fail, and it is not so
-		 * important, so make up a serial number and go on.
-		 */
-		mutex_lock(&steam->mutex);
-		if (steam_get_serial(steam) < 0)
-			strlcpy(steam->serial_no, "XXXXXXXXXX",
-					sizeof(steam->serial_no));
-		mutex_unlock(&steam->mutex);
+	if (steam->registered)
+		return 0;
 
-		hid_info(steam->hdev, "Steam Controller '%s' connected",
-				steam->serial_no);
+	/*
+	 * Unlikely, but getting the serial could fail, and it is not so
+	 * important, so make up a serial number and go on.
+	 */
+	mutex_lock(&steam->mutex);
+	if (steam_get_serial(steam) < 0 || !steam->serial_no[0])
+		strlcpy(steam->serial_no, "XXXXXXXXXX",
+				sizeof(steam->serial_no));
+	mutex_unlock(&steam->mutex);
 
-		/* ignore battery errors, we can live without it */
-		if (steam->quirks & STEAM_QUIRK_WIRELESS)
-			steam_battery_register(steam);
+	hid_info(steam->hdev, "Steam Controller '%s' connected",
+			steam->serial_no);
 
-		do_add = true;
-	}
+	/* ignore battery errors, we can live without it */
+	if (steam->quirks & STEAM_QUIRK_WIRELESS)
+		steam_battery_register(steam);
 
-	spin_lock_irqsave(&steam->lock, flags);
-	client_opened = steam->client_opened;
-	spin_unlock_irqrestore(&steam->lock, flags);
+	mutex_lock(&steam->mutex);
+	steam_set_lizard_mode(steam, lizard_mode);
+	mutex_unlock(&steam->mutex);
+	ret = steam_input_register(steam);
+	if (ret != 0)
+		goto steam_register_input_fail;
 
-	if (!client_opened) {
-		mutex_lock(&steam->mutex);
-		steam_set_lizard_mode(steam, lizard_mode);
-		mutex_unlock(&steam->mutex);
-		ret = steam_input_register(steam);
-		if (ret != 0)
-			goto steam_register_input_fail;
-	}
-
-	if (do_add) {
-		mutex_lock(&steam_devices_lock);
-		if (list_empty(&steam->list))
-			list_add(&steam->list, &steam_devices);
-		mutex_unlock(&steam_devices_lock);
-	}
+	steam->registered = true;
+	mutex_lock(&steam_devices_lock);
+	if (list_empty(&steam->list))
+		list_add(&steam->list, &steam_devices);
+	mutex_unlock(&steam_devices_lock);
 	return 0;
 
 steam_register_input_fail:
+	steam_battery_unregister(steam);
 	return ret;
 }
 
 static void steam_unregister(struct steam_device *steam)
 {
-	if (!steam->serial_no[0])
+	if (!steam->registered)
 		return;
 
 	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
 			steam->serial_no);
+	steam->registered = false;
 	steam_battery_unregister(steam);
 	steam_input_unregister(steam);
 	mutex_lock(&steam_devices_lock);
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [STABLE v5.15-v5.10 5/6] HID: steam: Refactor registration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: steam: initial connection status dropped due to missing hid_device_io_start
--

commit 06e6c231f0f1bba41bfa16d27899ea2f3bb451a9
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Refactor registration

This commit refactors and simplifies the registration/unregistration flow.
It relies on the client open state and a single boolean flag to manage
registration logic, and ensures the battery device is unregistered if later
initialization steps fail.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -739,20 +768,33 @@ static int steam_probe(struct hid_device *hdev,
[ ... ]
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver connected");
>  		/* If using a wireless adaptor ask for connection status */
>  		steam->connected = false;
>  		steam_request_conn_status(steam);
>  	} 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 input_register_fail;
> +			goto err_hw_close;
>  		}
>  	}
>  
> +	steam->client_hdev = steam_create_client_hid(hdev);
> +	if (IS_ERR(steam->client_hdev)) {
> +		ret = PTR_ERR(steam->client_hdev);
> +		goto err_stream_unregister;
> +	}
> +	steam->client_hdev->driver_data = steam;
> +
> +	ret = hid_add_device(steam->client_hdev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Will the earlier asynchronous status request be dropped because the HID core
input gate is still closed?

By moving the hid_add_device(steam->client_hdev) call to execute after
steam_request_conn_status(), the steam_probe() function takes longer to
return.

The hardware replies asynchronously with a STEAM_EV_CONNECT event on the
interrupt endpoint. Because hid_device_io_start() has not been called, the
HID core driver_input_lock remains held while the rest of steam_probe()
executes.

If the interrupt URB completes during this widened window, won't the HID
core unconditionally drop the connection status report, leaving the wireless
controller unresponsive?

> +	if (ret)
> +		goto err_destroy;
> +
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091215.529531-1-lee@kernel.org?part=5

[STABLE v5.15-v5.10 6/6] HID: steam: Clean up locking

[Lee Jones]

From: Vicki Pfau <vi@endrift.com>

[ Upstream commit de1d341b9b3ec9655f31fb8b44f784cca5e670ad ]

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

Signed-off-by: Vicki Pfau <vi@endrift.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
[Lee: Use explicit lock/unlock calls instead of guard(), adapt to
      steam->mutex, and omit Steam Deck sensors, mode_switch,
      rumble_work, coalesce_rumble_work and Ibex changes as
      691ead124a0c, e133dc99fc07, a0fbd9437d7d, 1d4549fd1115,
      fcfa7db89f7a and 0a80b4e8ec6a are not present]
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-steam.c | 61 +++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 544d45f3fc6b..4ba922d750df 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -114,6 +114,7 @@ struct steam_device {
 	spinlock_t lock;
 	struct hid_device *hdev, *client_hdev;
 	struct mutex mutex;
+	struct mutex registration_mutex;
 	unsigned long client_opened;
 	struct input_dev __rcu *input;
 	unsigned long quirks;
@@ -294,9 +295,14 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
 static int steam_input_open(struct input_dev *dev)
 {
 	struct steam_device *steam = input_get_drvdata(dev);
+	unsigned long flags;
+	bool client_opened;
 
+	spin_lock_irqsave(&steam->lock, flags);
+	client_opened = steam->client_opened;
+	spin_unlock_irqrestore(&steam->lock, flags);
 	mutex_lock(&steam->mutex);
-	if (!steam->client_opened && lizard_mode)
+	if (!client_opened && lizard_mode)
 		steam_set_lizard_mode(steam, false);
 	mutex_unlock(&steam->mutex);
 	return 0;
@@ -305,9 +311,14 @@ static int steam_input_open(struct input_dev *dev)
 static void steam_input_close(struct input_dev *dev)
 {
 	struct steam_device *steam = input_get_drvdata(dev);
+	unsigned long flags;
+	bool client_opened;
 
+	spin_lock_irqsave(&steam->lock, flags);
+	client_opened = steam->client_opened;
+	spin_unlock_irqrestore(&steam->lock, flags);
 	mutex_lock(&steam->mutex);
-	if (!steam->client_opened && lizard_mode)
+	if (!client_opened && lizard_mode)
 		steam_set_lizard_mode(steam, true);
 	mutex_unlock(&steam->mutex);
 }
@@ -513,40 +524,43 @@ 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.
 	 * The battery and serial number are set just once per device.
 	 */
-	if (steam->registered)
+	if (steam->registered) {
+		mutex_unlock(&steam->registration_mutex);
 		return 0;
+	}
 
+	mutex_lock(&steam->mutex);
 	/*
 	 * Unlikely, but getting the serial could fail, and it is not so
 	 * important, so make up a serial number and go on.
 	 */
-	mutex_lock(&steam->mutex);
 	if (steam_get_serial(steam) < 0 || !steam->serial_no[0])
 		strlcpy(steam->serial_no, "XXXXXXXXXX",
 				sizeof(steam->serial_no));
-	mutex_unlock(&steam->mutex);
 
 	hid_info(steam->hdev, "Steam Controller '%s' connected",
 			steam->serial_no);
 
+	steam_set_lizard_mode(steam, lizard_mode);
+	mutex_unlock(&steam->mutex);
+
 	/* ignore battery errors, we can live without it */
 	if (steam->quirks & STEAM_QUIRK_WIRELESS)
 		steam_battery_register(steam);
 
-	mutex_lock(&steam->mutex);
-	steam_set_lizard_mode(steam, lizard_mode);
-	mutex_unlock(&steam->mutex);
 	ret = steam_input_register(steam);
 	if (ret != 0)
 		goto steam_register_input_fail;
 
 	steam->registered = true;
+	mutex_unlock(&steam->registration_mutex);
 	mutex_lock(&steam_devices_lock);
 	if (list_empty(&steam->list))
 		list_add(&steam->list, &steam_devices);
@@ -555,6 +569,7 @@ static int steam_register(struct steam_device *steam)
 
 steam_register_input_fail:
 	steam_battery_unregister(steam);
+	mutex_unlock(&steam->registration_mutex);
 	return ret;
 }
 
@@ -565,9 +580,11 @@ static void steam_unregister(struct steam_device *steam)
 
 	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
 			steam->serial_no);
+	mutex_lock(&steam->registration_mutex);
 	steam->registered = false;
 	steam_battery_unregister(steam);
 	steam_input_unregister(steam);
+	mutex_unlock(&steam->registration_mutex);
 	mutex_lock(&steam_devices_lock);
 	list_del_init(&steam->list);
 	mutex_unlock(&steam_devices_lock);
@@ -745,6 +762,7 @@ static int steam_probe(struct hid_device *hdev,
 	hid_set_drvdata(hdev, steam);
 	spin_lock_init(&steam->lock);
 	mutex_init(&steam->mutex);
+	mutex_init(&steam->registration_mutex);
 	steam->quirks = id->driver_data;
 	INIT_WORK(&steam->work_connect, steam_work_connect_cb);
 	INIT_LIST_HEAD(&steam->list);
@@ -835,13 +853,10 @@ static void steam_remove(struct hid_device *hdev)
 
 static void steam_do_connect_event(struct steam_device *steam, bool connected)
 {
-	unsigned long flags;
 	bool changed;
 
-	spin_lock_irqsave(&steam->lock, flags);
 	changed = steam->connected != connected;
 	steam->connected = connected;
-	spin_unlock_irqrestore(&steam->lock, flags);
 
 	if (changed && schedule_work(&steam->work_connect) == 0)
 		dbg_hid("%s: connected=%d event already queued\n",
@@ -1008,8 +1023,6 @@ static void steam_do_input_event(struct steam_device *steam,
 static void steam_do_battery_event(struct steam_device *steam,
 		struct power_supply *battery, u8 *data)
 {
-	unsigned long flags;
-
 	s16 volts = steam_le16(data + 12);
 	u8 batt = data[14];
 
@@ -1017,10 +1030,8 @@ static void steam_do_battery_event(struct steam_device *steam,
 	rcu_read_lock();
 	battery = rcu_dereference(steam->battery);
 	if (likely(battery)) {
-		spin_lock_irqsave(&steam->lock, flags);
 		steam->voltage = volts;
 		steam->battery_charge = batt;
-		spin_unlock_irqrestore(&steam->lock, flags);
 		power_supply_changed(battery);
 	}
 	rcu_read_unlock();
@@ -1033,10 +1044,12 @@ static int steam_raw_event(struct hid_device *hdev,
 	struct steam_device *steam = hid_get_drvdata(hdev);
 	struct input_dev *input;
 	struct power_supply *battery;
+	unsigned long flags;
 
 	if (!steam)
 		return 0;
 
+	spin_lock_irqsave(&steam->lock, flags);
 	if (steam->client_opened)
 		hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
 				data, size, 0);
@@ -1057,12 +1070,12 @@ static int steam_raw_event(struct hid_device *hdev,
 	 */
 
 	if (size != 64 || data[0] != 1 || data[1] != 0)
-		return 0;
+		goto out;
 
 	switch (data[2]) {
 	case STEAM_EV_INPUT_DATA:
 		if (steam->client_opened)
-			return 0;
+			goto out;
 		rcu_read_lock();
 		input = rcu_dereference(steam->input);
 		if (likely(input))
@@ -1100,6 +1113,8 @@ static int steam_raw_event(struct hid_device *hdev,
 		}
 		break;
 	}
+out:
+	spin_unlock_irqrestore(&steam->lock, flags);
 	return 0;
 }
 
@@ -1108,6 +1123,8 @@ static int steam_param_set_lizard_mode(const char *val,
 {
 	struct steam_device *steam;
 	int ret;
+	bool client_opened;
+	unsigned long flags;
 
 	ret = param_set_bool(val, kp);
 	if (ret)
@@ -1115,10 +1132,14 @@ static int steam_param_set_lizard_mode(const char *val,
 
 	mutex_lock(&steam_devices_lock);
 	list_for_each_entry(steam, &steam_devices, list) {
-		mutex_lock(&steam->mutex);
-		if (!steam->client_opened)
+		spin_lock_irqsave(&steam->lock, flags);
+		client_opened = steam->client_opened;
+		spin_unlock_irqrestore(&steam->lock, flags);
+		if (!client_opened) {
+			mutex_lock(&steam->mutex);
 			steam_set_lizard_mode(steam, lizard_mode);
-		mutex_unlock(&steam->mutex);
+			mutex_unlock(&steam->mutex);
+		}
 	}
 	mutex_unlock(&steam_devices_lock);
 	return 0;
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [STABLE v5.15-v5.10 6/6] HID: steam: Clean up locking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: steam: TOCTOU race in input_open/close corrupts lizard mode
--

commit ad9df30996ddcb8a183a74dda605518707156658
Author: Vicki Pfau <vi@endrift.com>
Date:   Tue Oct 13 09:12:13 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 5414f446753de..df21c48928303 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -284,9 +285,14 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
>  static int steam_input_open(struct input_dev *dev)
>  {
>  	struct steam_device *steam = input_get_drvdata(dev);
> +	unsigned long flags;
> +	bool client_opened;
>  
> +	spin_lock_irqsave(&steam->lock, flags);
> +	client_opened = steam->client_opened;
> +	spin_unlock_irqrestore(&steam->lock, flags);
>  	mutex_lock(&steam->mutex);
> -	if (!steam->client_opened && lizard_mode)
> +	if (!client_opened && lizard_mode)
>  		steam_set_lizard_mode(steam, false);
>  	mutex_unlock(&steam->mutex);
>  	return 0;

[Severity: Medium]
Could this introduce a TOCTOU race condition? We read steam->client_opened 
under steam->lock into a local variable, drop the lock, and then block on 
steam->mutex. 

Concurrently, a raw hidraw client can open or close the device, which modifies
steam->client_opened under steam->lock. Does this allow the function to act
on stale state while holding the mutex and leave lizard mode incorrectly enabled?

>  }
> @@ -295,9 +301,14 @@ static int steam_input_open(struct input_dev *dev)
>  static void steam_input_close(struct input_dev *dev)
>  {
>  	struct steam_device *steam = input_get_drvdata(dev);
> +	unsigned long flags;
> +	bool client_opened;
>  
> +	spin_lock_irqsave(&steam->lock, flags);
> +	client_opened = steam->client_opened;
> +	spin_unlock_irqrestore(&steam->lock, flags);
>  	mutex_lock(&steam->mutex);
> -	if (!steam->client_opened && lizard_mode)
> +	if (!client_opened && lizard_mode)
>  		steam_set_lizard_mode(steam, true);
>  	mutex_unlock(&steam->mutex);
>  }

[Severity: Medium]
Similar to steam_input_open(), does this TOCTOU window allow
steam_input_close() to incorrectly turn lizard mode ON just as a concurrent
raw hidraw client is opening the device and attempting to manage it? 

Since the Steam client only sends its initialization commands (such as
disabling lizard mode) once upon startup and does not continuously poll, it
seems it would not fix a spurious state change forced by the driver behind
its back.

[ ... ]
> @@ -1098,6 +1113,8 @@ static int steam_param_set_lizard_mode(const char *val,
>  {
>  	struct steam_device *steam;
>  	int ret;
> +	bool client_opened;
> +	unsigned long flags;
>  
>  	ret = param_set_bool(val, kp);
>  	if (ret)
> @@ -1105,10 +1122,14 @@ static int steam_param_set_lizard_mode(const char *val,
>  
>  	mutex_lock(&steam_devices_lock);
>  	list_for_each_entry(steam, &steam_devices, list) {
> -		mutex_lock(&steam->mutex);
> -		if (!steam->client_opened)
> +		spin_lock_irqsave(&steam->lock, flags);
> +		client_opened = steam->client_opened;
> +		spin_unlock_irqrestore(&steam->lock, flags);
> +		if (!client_opened) {
> +			mutex_lock(&steam->mutex);
>  			steam_set_lizard_mode(steam, lizard_mode);
> -		mutex_unlock(&steam->mutex);
> +			mutex_unlock(&steam->mutex);
> +		}
>  	}
>  	mutex_unlock(&steam_devices_lock);
>  	return 0;

[Severity: Medium]
Does this have the same TOCTOU race with steam->client_opened as 
steam_input_open() and steam_input_close() when setting the lizard mode
parameter via sysfs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091215.529531-1-lee@kernel.org?part=6

Re: [STABLE v5.15-v5.10 1/6] HID: hid-steam: Fix cleanup in probe()

[Sasha Levin]

> There are a number of issues in this code.  First of all if
> steam_create_client_hid() fails then it leads to an error pointer
> dereference when we call hid_destroy_device(steam->client_hdev).

Queued the series for 5.15 and 5.10, thanks.

-- 
Thanks,
Sasha