[PATCH v5 00/10] HID: steam: General cleanup and improvements

[PATCH v5 00/10] HID: steam: General cleanup and improvements

[Vicki Pfau]

This is the first half of a patch series improving the hid-steam driver.

This series contains a few notable changes:
- As I've been maintaining it downstream in SteamOS as well as writing
  general improvements, I'm marking myself as the maintainer of the driver.
- Renames some stuff, notably marking the original Steam Controller as
  Steam Controller (2015) in preparation of adding support for the Steam
  Controller (2026), which will be in the second half of this series once
  it's ready for submission.
- Adds IMU support for the Steam Controller (2015).
- Some reliability improvements, especially surrounding edge cases
  regarding scheduled work during teardown.
- Fixes a short read issue reported by syzkaller.

Hopefully the second half of the series will be ready soon, but I am
waiting on more testing by SteamOS users before I submit it.

v4 introduced failed to properly revert a patch from v3, which is fixed
here, as well as a completely random other bug that sashiko brought up for
some reason.

I am pretty exhausted from sashiko jerking me around. It has several issues
that I think are entirely imagined, in this series mostly invovling HID
startup and teardown. It seems to believe that HID interrupts will get
serviced during startup and teardown, but I'm pretty sure that isn't the
case if hid_device_io_start isn't called. I've wasted a lot of hours trying
to fix theoretical issues regarding this in this and other patch series.
And that's only one of the classes of bugs it's hallucinated. This thing is
very clearly not ready to be taken seriously, especially if we don't want
to burn out maintainers.

Vicki Pfau (11):
  HID: steam: Update documentation
  HID: steam: Refactor and clean up report parsing
  HID: steam: Rename some constants that got renamed upstream
  HID: steam: Add support for sensor events on the Steam Controller
    (2015)
  HID: steam: Coalesce rumble packets
  HID: steam: Fully unregister controller when hidraw is opened
  HID: steam: Rearrange teardown sequence
  HID: steam: Improve logging and other cleanup
  HID: steam: Zero-initialize reply in serial lookup
  HID: steam: Reject short reads
  HID: steam: Retry send/recv reports if stale

 MAINTAINERS             |   6 +
 drivers/hid/hid-steam.c | 685 +++++++++++++++++++++++++++-------------
 2 files changed, 473 insertions(+), 218 deletions(-)

-- 
2.54.0

[PATCH v5 01/11] HID: steam: Update documentation

[Vicki Pfau]

Mark myself as the maintainer, as well as adding myself as an author.

It also makes some minor updates to comments, such as correcly calling the
left menu key view and retroactively renaming the original Steam Controller
as Steam Controller (2015), in preparation for support for the 2026 model.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 MAINTAINERS             |  6 ++++++
 drivers/hid/hid-steam.c | 13 +++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index f37a81950e25..8f2c582f77e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11566,6 +11566,12 @@ F:	drivers/hid/hid-sensor-*
 F:	drivers/iio/*/hid-*
 F:	include/linux/hid-sensor-*
 
+HID STEAM CONTROLLER
+M:	Vicki Pfau <vi@endrift.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/hid/hid-steam.c
+
 HID VRC-2 CAR CONTROLLER DRIVER
 M:	Marcus Folkesson <marcus.folkesson@gmail.com>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 197126d6e081..a854d6360a0e 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -48,6 +48,7 @@
 MODULE_DESCRIPTION("HID driver for Valve Steam Controller");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+MODULE_AUTHOR("Vicki Pfau <vi@endrift.com>");
 
 static bool lizard_mode = true;
 
@@ -1413,9 +1414,9 @@ static inline s16 steam_le16(u8 *data)
  *  9.1  | BTN_DPAD_RIGHT | left-pad right
  *  9.2  | BTN_DPAD_LEFT  | left-pad left
  *  9.3  | BTN_DPAD_DOWN  | left-pad down
- *  9.4  | BTN_SELECT | menu left
+ *  9.4  | BTN_SELECT | view
  *  9.5  | BTN_MODE   | steam logo
- *  9.6  | BTN_START  | menu right
+ *  9.6  | BTN_START  | menu
  *  9.7  | BTN_GRIPL  | left back lever
  * 10.0  | BTN_GRIPR  | right back lever
  * 10.1  | --         | left-pad clicked
@@ -1541,9 +1542,9 @@ static void steam_do_input_event(struct steam_device *steam,
  *  9.1  | BTN_DPAD_RIGHT | left-pad right
  *  9.2  | BTN_DPAD_LEFT  | left-pad left
  *  9.3  | BTN_DPAD_DOWN  | left-pad down
- *  9.4  | BTN_SELECT | menu left
+ *  9.4  | BTN_SELECT | view
  *  9.5  | BTN_MODE   | steam logo
- *  9.6  | BTN_START  | menu right
+ *  9.6  | BTN_START  | menu
  *  9.7  | BTN_GRIPL2 | left bottom grip button
  *  10.0 | BTN_GRIPR2 | right bottom grip button
  *  10.1 | BTN_THUMB  | left pad pressed
@@ -1850,11 +1851,11 @@ MODULE_PARM_DESC(lizard_mode,
 	"Enable mouse and keyboard emulation (lizard mode) when the gamepad is not in use");
 
 static const struct hid_device_id steam_controllers[] = {
-	{ /* Wired Steam Controller */
+	{ /* Wired Steam Controller (2015) */
 	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 		USB_DEVICE_ID_STEAM_CONTROLLER)
 	},
-	{ /* Wireless Steam Controller */
+	{ /* Wireless Steam Controller (2015) */
 	  HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
 		USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
 	  .driver_data = STEAM_QUIRK_WIRELESS
-- 
2.54.0

[PATCH v5 02/11] HID: steam: Refactor and clean up report parsing

[Vicki Pfau]

This switches from a parsing style where each button or axis is parsed
individually out of a report using !!(byte & BIT(x)) style. This commit
switches it to a mostly unified approach of defining a list of individual
mappings in an array and passing it to a function that handles all of the
extraction. Theoretically this is more lines, but in practice it results in
(subjectively) cleaner code. Some exceptions still need to be made for
things like handling the lizard mode toggle key, but in general there's a
lot less manual code.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 211 ++++++++++++++++++++++++----------------
 1 file changed, 128 insertions(+), 83 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index a854d6360a0e..75d6be0be0a2 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -43,6 +43,7 @@
 #include <linux/rcupdate.h>
 #include <linux/delay.h>
 #include <linux/power_supply.h>
+#include <linux/unaligned.h>
 #include "hid-ids.h"
 
 MODULE_DESCRIPTION("HID driver for Valve Steam Controller");
@@ -1355,13 +1356,45 @@ static void steam_do_connect_event(struct steam_device *steam, bool connected)
  * Clamp the values to 32767..-32767 so that the range is
  * symmetrical and can be negated safely.
  */
-static inline s16 steam_le16(u8 *data)
+static inline s16 steam_le16(const u8 *data)
 {
-	s16 x = (s16) le16_to_cpup((__le16 *)data);
+	s16 x = (s16) get_unaligned_le16((const __le16 *)data);
 
 	return x == -32768 ? -32767 : x;
 }
 
+struct steam_button_mapping {
+	int code;
+	u8 byte;
+	u8 bit;
+};
+
+struct steam_axis_mapping {
+	int code;
+	s8 sign;
+	u8 byte;
+};
+
+static void steam_map_buttons(struct input_dev *input,
+		const struct steam_button_mapping *mappings, const u8 *data)
+{
+	const struct steam_button_mapping *mapping;
+
+	for (mapping = mappings; mapping->code; mapping++)
+		input_report_key(input, mapping->code,
+			data[mapping->byte] & BIT(mapping->bit));
+}
+
+static void steam_map_axes(struct input_dev *input,
+		const struct steam_axis_mapping *mappings, const u8 *data)
+{
+	const struct steam_axis_mapping *mapping;
+
+	for (mapping = mappings; mapping->sign; mapping++)
+		input_report_abs(input, mapping->code,
+			mapping->sign * steam_le16(&data[mapping->byte]));
+}
+
 /*
  * The size for this message payload is 60.
  * The known values are:
@@ -1428,18 +1461,42 @@ static inline s16 steam_le16(u8 *data)
  * 10.7  | --         | lpad_and_joy
  */
 
+static const struct steam_button_mapping steam_controller_button_mappings[] = {
+	{ BTN_TR2,		 8, 0 },
+	{ BTN_TL2,		 8, 1 },
+	{ BTN_TR,		 8, 2 },
+	{ BTN_TL,		 8, 3 },
+	{ BTN_Y,		 8, 4 },
+	{ BTN_B,		 8, 5 },
+	{ BTN_X,		 8, 6 },
+	{ BTN_A,		 8, 7 },
+	{ BTN_SELECT,		 9, 4 },
+	{ BTN_MODE,		 9, 5 },
+	{ BTN_START,		 9, 6 },
+	{ BTN_GRIPL,		 9, 7 },
+	{ BTN_GRIPR,		10, 0 },
+	{ BTN_THUMBR,		10, 2 },
+	{ BTN_THUMBL,		10, 6 },
+	{ BTN_THUMB2,		10, 4 },
+	{ BTN_DPAD_UP,		 9, 0 },
+	{ BTN_DPAD_RIGHT,	 9, 1 },
+	{ BTN_DPAD_LEFT,	 9, 2 },
+	{ BTN_DPAD_DOWN,	 9, 3 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_controller_axis_mappings[] = {
+	{ ABS_RX,  1, 20 },
+	{ ABS_RY, -1, 22 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
-	/* 24 bits of buttons */
-	u8 b8, b9, b10;
 	s16 x, y;
 	bool lpad_touched, lpad_and_joy;
 
-	b8 = data[8];
-	b9 = data[9];
-	b10 = data[10];
-
 	input_report_abs(input, ABS_HAT2Y, data[11]);
 	input_report_abs(input, ABS_HAT2X, data[12]);
 
@@ -1451,8 +1508,8 @@ static void steam_do_input_event(struct steam_device *steam,
 	 * joystick values.
 	 * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
 	 */
-	lpad_touched = b10 & BIT(3);
-	lpad_and_joy = b10 & BIT(7);
+	lpad_touched = data[10] & BIT(3);
+	lpad_and_joy = data[10] & BIT(7);
 	x = steam_le16(data + 16);
 	y = -steam_le16(data + 18);
 
@@ -1468,31 +1525,10 @@ static void steam_do_input_event(struct steam_device *steam,
 		input_report_abs(input, ABS_HAT0X, 0);
 		input_report_abs(input, ABS_HAT0Y, 0);
 	}
+	input_report_key(input, BTN_THUMB, lpad_touched || lpad_and_joy);
 
-	input_report_abs(input, ABS_RX, steam_le16(data + 20));
-	input_report_abs(input, ABS_RY, -steam_le16(data + 22));
-
-	input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
-	input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
-	input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
-	input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
-	input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
-	input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
-	input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
-	input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
-	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
-	input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
-	input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
-	input_event(input, EV_KEY, BTN_GRIPL, !!(b9 & BIT(7)));
-	input_event(input, EV_KEY, BTN_GRIPR, !!(b10 & BIT(0)));
-	input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
-	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
-	input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
-	input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
-	input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
-	input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
-	input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
+	steam_map_buttons(input, steam_controller_button_mappings, data);
+	steam_map_axes(input, steam_controller_axis_mappings, data);
 
 	input_sync(input);
 }
@@ -1595,23 +1631,67 @@ static void steam_do_input_event(struct steam_device *steam,
  *  15.6 | --         | unknown
  *  15.7 | --         | unknown
  */
+
+static const struct steam_button_mapping steam_deck_button_mappings[] = {
+	{ BTN_TR2,		 8, 0 },
+	{ BTN_TL2,		 8, 1 },
+	{ BTN_TR,		 8, 2 },
+	{ BTN_TL,		 8, 3 },
+	{ BTN_Y,		 8, 4 },
+	{ BTN_B,		 8, 5 },
+	{ BTN_X,		 8, 6 },
+	{ BTN_A,		 8, 7 },
+	{ BTN_SELECT,		 9, 4 },
+	{ BTN_MODE,		 9, 5 },
+	{ BTN_START,		 9, 6 },
+	{ BTN_GRIPL2,		 9, 7 },
+	{ BTN_GRIPR2,		10, 0 },
+	{ BTN_THUMBL,		10, 6 },
+	{ BTN_THUMBR,		11, 2 },
+	{ BTN_DPAD_UP,		 9, 0 },
+	{ BTN_DPAD_RIGHT,	 9, 1 },
+	{ BTN_DPAD_LEFT,	 9, 2 },
+	{ BTN_DPAD_DOWN,	 9, 3 },
+	{ BTN_THUMB,		10, 1 },
+	{ BTN_THUMB2,		10, 2 },
+	{ BTN_GRIPL,		13, 1 },
+	{ BTN_GRIPR,		13, 2 },
+	{ BTN_BASE,		14, 2 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_deck_axis_mappings[] = {
+	{ ABS_X,	 1, 48 },
+	{ ABS_Y,	-1, 50 },
+	{ ABS_RX,	 1, 52 },
+	{ ABS_RY,	-1, 54 },
+	{ ABS_HAT2Y,	 1, 44 },
+	{ ABS_HAT2X,	 1, 46 },
+	{ /* sentinel */ },
+};
+
+static const struct steam_axis_mapping steam_deck_imu_mappings[] = {
+	{ ABS_X,   1, 24 },
+	{ ABS_Z,  -1, 26 },
+	{ ABS_Y,   1, 28 },
+	{ ABS_RX,  1, 30 },
+	{ ABS_RZ, -1, 32 },
+	{ ABS_RY,  1, 34 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_deck_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
-	u8 b8, b9, b10, b11, b13, b14;
+	bool start_pressed;
 	bool lpad_touched, rpad_touched;
 
-	b8 = data[8];
-	b9 = data[9];
-	b10 = data[10];
-	b11 = data[11];
-	b13 = data[13];
-	b14 = data[14];
+	start_pressed = data[9] & BIT(6);
 
-	if (!(b9 & BIT(6)) && steam->did_mode_switch) {
+	if (!start_pressed && steam->did_mode_switch) {
 		steam->did_mode_switch = false;
 		cancel_delayed_work(&steam->mode_switch);
-	} else if (!steam->client_opened && (b9 & BIT(6)) && !steam->did_mode_switch) {
+	} else if (!steam->client_opened && start_pressed && !steam->did_mode_switch) {
 		steam->did_mode_switch = true;
 		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
 	}
@@ -1619,8 +1699,8 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 	if (!steam->gamepad_mode && lizard_mode)
 		return;
 
-	lpad_touched = b10 & BIT(3);
-	rpad_touched = b10 & BIT(4);
+	lpad_touched = data[10] & BIT(3);
+	rpad_touched = data[10] & BIT(4);
 
 	if (lpad_touched) {
 		input_report_abs(input, ABS_HAT0X, steam_le16(data + 16));
@@ -1638,38 +1718,8 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 		input_report_abs(input, ABS_HAT1Y, 0);
 	}
 
-	input_report_abs(input, ABS_X, steam_le16(data + 48));
-	input_report_abs(input, ABS_Y, -steam_le16(data + 50));
-	input_report_abs(input, ABS_RX, steam_le16(data + 52));
-	input_report_abs(input, ABS_RY, -steam_le16(data + 54));
-
-	input_report_abs(input, ABS_HAT2Y, steam_le16(data + 44));
-	input_report_abs(input, ABS_HAT2X, steam_le16(data + 46));
-
-	input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
-	input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
-	input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
-	input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
-	input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
-	input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
-	input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
-	input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
-	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
-	input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
-	input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
-	input_event(input, EV_KEY, BTN_GRIPL2, !!(b9 & BIT(7)));
-	input_event(input, EV_KEY, BTN_GRIPR2, !!(b10 & BIT(0)));
-	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
-	input_event(input, EV_KEY, BTN_THUMBR, !!(b11 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
-	input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
-	input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
-	input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
-	input_event(input, EV_KEY, BTN_THUMB, !!(b10 & BIT(1)));
-	input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(2)));
-	input_event(input, EV_KEY, BTN_GRIPL, !!(b13 & BIT(1)));
-	input_event(input, EV_KEY, BTN_GRIPR, !!(b13 & BIT(2)));
-	input_event(input, EV_KEY, BTN_BASE, !!(b14 & BIT(2)));
+	steam_map_buttons(input, steam_deck_button_mappings, data);
+	steam_map_axes(input, steam_deck_axis_mappings, data);
 
 	input_sync(input);
 }
@@ -1690,12 +1740,7 @@ static void steam_do_deck_sensors_event(struct steam_device *steam,
 		return;
 
 	input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
-	input_report_abs(sensors, ABS_X, steam_le16(data + 24));
-	input_report_abs(sensors, ABS_Z, -steam_le16(data + 26));
-	input_report_abs(sensors, ABS_Y, steam_le16(data + 28));
-	input_report_abs(sensors, ABS_RX, steam_le16(data + 30));
-	input_report_abs(sensors, ABS_RZ, -steam_le16(data + 32));
-	input_report_abs(sensors, ABS_RY, steam_le16(data + 34));
+	steam_map_axes(sensors, steam_deck_imu_mappings, data);
 
 	input_sync(sensors);
 }
-- 
2.54.0

Re: [PATCH v5 02/11] HID: steam: Refactor and clean up report parsing

[sashiko-bot]

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

Pre-existing issues:
- [High] Works (`work_connect` and `rumble_work`) can be re-armed after they are cancelled during device removal, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH v5 02/11] HID: steam: Refactor and clean up report parsing ---
commit b341becd37dc80cfee5384e2c463afc0e1624d32
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Refactor and clean up report parsing

This switches from a parsing style where each button or axis is parsed
individually out of a report using !!(byte & BIT(x)) style. This commit
switches it to a mostly unified approach of defining a list of individual
mappings in an array and passing it to a function that handles all of the
extraction. Theoretically this is more lines, but in practice it results in
(subjectively) cleaner code. Some exceptions still need to be made for
things like handling the lizard mode toggle key, but in general there's a
lot less manual code.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a854d6360a0e3..75d6be0be0a29 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -43,6 +43,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/delay.h>
>  #include <linux/power_supply.h>
> +#include <linux/unaligned.h>
>  #include "hid-ids.h"
>  
>  MODULE_DESCRIPTION("HID driver for Valve Steam Controller");

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing the driver's event handling 
I noticed a potential use-after-free during device removal.

In steam_remove(), work_connect is cancelled before the hardware event 
source is stopped:

steam_remove() {
    ...
    cancel_work_sync(&steam->work_connect);
    if (steam->quirks & STEAM_QUIRK_WIRELESS) {
        hid_info(hdev, "Steam wireless receiver disconnected");
    }
    steam_unregister(steam);
    hid_hw_stop(hdev);
}

If a wireless connect or disconnect report arrives after cancel_work_sync() 
but before hid_hw_stop(), steam_do_connect_event() could process it:

steam_do_connect_event() {
    ...
    if (changed && schedule_work(&steam->work_connect) == 0)
    ...
}

Could this re-arm the cancelled work, leading to it executing after the 
devm-managed steam_device is freed? Would it be safer to call hid_hw_stop() 
earlier in the function?

[Severity: High]
This is also a pre-existing issue, but I noticed a similar work queue ordering
issue in steam_unregister().

The rumble_work is cancelled before its producer, coalesce_rumble_work:

steam_unregister() {
    ...
    cancel_work_sync(&steam->rumble_work);
    cancel_delayed_work_sync(&steam->mode_switch);
    cancel_delayed_work_sync(&steam->coalesce_rumble_work);
    ...
}

If coalesce_rumble_work fires during this window, its callback
steam_coalesce_rumble_cb() unconditionally calls 
schedule_work(&steam->rumble_work). 

Could this cause the rumble_work to be re-armed and subsequently execute on 
freed memory? Would reversing the cancellation order prevent this race?

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

[PATCH v5 03/11] HID: steam: Rename some constants that got renamed upstream

[Vicki Pfau]

SETTING_MOUSE_POINTER_ENABLED was renamed to SETTING_LIZARD_MODE upstream.
SETTING_GYRO_MODE was renamed to SETTING_IMU_MODE in an older commit, but
the associated enum was overlooked.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 75d6be0be0a2..983d18d1de4f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -151,7 +151,7 @@ enum {
 	SETTING_USB_DEBUG_MODE,
 	SETTING_LEFT_TRACKPAD_MODE,
 	SETTING_RIGHT_TRACKPAD_MODE,
-	SETTING_MOUSE_POINTER_ENABLED,
+	SETTING_LIZARD_MODE,
 
 	/* 10 */
 	SETTING_DPAD_DEADZONE,
@@ -261,14 +261,14 @@ enum {
 	ATTRIB_STR_UNIT_SERIAL,
 };
 
-/* Values for GYRO_MODE (bitmask) */
+/* Values for IMU_MODE (bitmask) */
 enum {
-	SETTING_GYRO_MODE_OFF			= 0,
-	SETTING_GYRO_MODE_STEERING		= BIT(0),
-	SETTING_GYRO_MODE_TILT			= BIT(1),
-	SETTING_GYRO_MODE_SEND_ORIENTATION	= BIT(2),
-	SETTING_GYRO_MODE_SEND_RAW_ACCEL	= BIT(3),
-	SETTING_GYRO_MODE_SEND_RAW_GYRO		= BIT(4),
+	SETTING_IMU_MODE_OFF			= 0,
+	SETTING_IMU_MODE_STEERING		= BIT(0),
+	SETTING_IMU_MODE_TILT			= BIT(1),
+	SETTING_IMU_MODE_SEND_ORIENTATION	= BIT(2),
+	SETTING_IMU_MODE_SEND_RAW_ACCEL		= BIT(3),
+	SETTING_IMU_MODE_SEND_RAW_GYRO		= BIT(4),
 };
 
 /* Trackpad modes */
-- 
2.54.0

[PATCH v5 04/11] HID: steam: Add support for sensor events on the Steam Controller (2015)

[Vicki Pfau]

Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 214 ++++++++++++++++++++++++++++++++--------
 1 file changed, 175 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 983d18d1de4f..1ceb044e170b 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -70,13 +70,14 @@ static LIST_HEAD(steam_devices);
 /* Joystick runs are about 5 mm and 32768 units */
 #define STEAM_DECK_JOYSTICK_RESOLUTION 6553
 /* Accelerometer has 16 bit resolution and a range of +/- 2g */
-#define STEAM_DECK_ACCEL_RES_PER_G 16384
-#define STEAM_DECK_ACCEL_RANGE 32768
+#define STEAM_ACCEL_RES_PER_G 16384
+#define STEAM_ACCEL_RANGE 32768
+#define STEAM_ACCEL_FUZZ 128
 #define STEAM_DECK_ACCEL_FUZZ 32
 /* Gyroscope has 16 bit resolution and a range of +/- 2000 dps */
-#define STEAM_DECK_GYRO_RES_PER_DPS 16
-#define STEAM_DECK_GYRO_RANGE 32768
-#define STEAM_DECK_GYRO_FUZZ 1
+#define STEAM_GYRO_RES_PER_DPS 16
+#define STEAM_GYRO_RANGE 32768
+#define STEAM_GYRO_FUZZ 0
 
 #define STEAM_PAD_FUZZ 256
 
@@ -255,6 +256,31 @@ enum
 	ID_CONTROLLER_DECK_STATE = 9
 };
 
+/* Read-only attributes */
+enum {
+	ATTRIB_UNIQUE_ID, // deprecated
+	ATTRIB_PRODUCT_ID,
+	ATTRIB_PRODUCT_REVISON, // deprecated
+	ATTRIB_CAPABILITIES = ATTRIB_PRODUCT_REVISON, // intentional aliasing
+	ATTRIB_FIRMWARE_VERSION, // deprecated
+	ATTRIB_FIRMWARE_BUILD_TIME,
+	ATTRIB_RADIO_FIRMWARE_BUILD_TIME,
+	ATTRIB_RADIO_DEVICE_ID0,
+	ATTRIB_RADIO_DEVICE_ID1,
+	ATTRIB_DONGLE_FIRMWARE_BUILD_TIME,
+	ATTRIB_HW_ID, // AKA BOARD_REVISION,
+	ATTRIB_BOOTLOADER_BUILD_TIME,
+	ATTRIB_CONNECTION_INTERVAL_IN_US,
+	ATTRIB_SECONDARY_FIRMWARE_BUILD_TIME,
+	ATTRIB_SECONDARY_BOOTLOADER_BUILD_TIME,
+	ATTRIB_SECONDARY_HW_ID, // AKA BOARD_REVISION,
+	ATTRIB_STREAMING,
+	ATTRIB_TRACKPAD_ID,
+	ATTRIB_SECONDARY_TRACKPAD_ID,
+
+	ATTRIB_COUNT
+};
+
 /* String attribute identifiers */
 enum {
 	ATTRIB_STR_BOARD_SERIAL,
@@ -284,6 +310,11 @@ enum {
 	TRACKPAD_GESTURE_KEYBOARD,
 };
 
+struct steam_controller_attribute {
+	unsigned char tag;
+	__le32 value;
+} __packed;
+
 /* Pad identifiers for the deck */
 #define STEAM_PAD_LEFT 0
 #define STEAM_PAD_RIGHT 1
@@ -315,6 +346,7 @@ struct steam_device {
 	u16 rumble_left;
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
+	unsigned int sensor_update_rate_us;
 	struct work_struct unregister_work;
 };
 
@@ -468,6 +500,38 @@ static int steam_get_serial(struct steam_device *steam)
 	return ret;
 }
 
+static int steam_get_attributes(struct steam_device *steam)
+{
+	int ret = 0;
+	u8 cmd[] = {ID_GET_ATTRIBUTES_VALUES, 0};
+	u8 reply[64] = {};
+	u8 size;
+	int i;
+	struct steam_controller_attribute *attr;
+
+	guard(mutex)(&steam->report_mutex);
+	ret = steam_send_report(steam, cmd, sizeof(cmd));
+	if (ret < 0)
+		return ret;
+	ret = steam_recv_report(steam, reply, sizeof(reply));
+	if (ret < 0)
+		return ret;
+	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
+		return -EIO;
+
+	size = min(reply[1], sizeof(reply) - 2);
+	for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
+		attr = (struct steam_controller_attribute *)&reply[i + 2];
+		if (attr->tag == ATTRIB_CONNECTION_INTERVAL_IN_US) {
+			steam->sensor_update_rate_us = get_unaligned_le32(&attr->value);
+			hid_dbg(steam->hdev, "Sensor update rate: %uus\n",
+				steam->sensor_update_rate_us);
+		}
+	}
+
+	return 0;
+}
+
 /*
  * This command requests the wireless adaptor to post an event
  * with the connection status. Useful if this driver is loaded when
@@ -626,6 +690,42 @@ static void steam_input_close(struct input_dev *dev)
 	}
 }
 
+static int steam_sensor_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);
+	if (client_opened)
+		return 0;
+
+	guard(mutex)(&steam->report_mutex);
+	steam_write_settings(steam, SETTING_IMU_MODE,
+			SETTING_IMU_MODE_SEND_RAW_ACCEL | SETTING_IMU_MODE_SEND_RAW_GYRO,
+			0);
+
+	return 0;
+}
+
+static void steam_sensor_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);
+	if (client_opened)
+		return;
+
+	guard(mutex)(&steam->report_mutex);
+	steam_write_settings(steam, SETTING_IMU_MODE, 0, 0);
+}
+
 static enum power_supply_property steam_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_SCOPE,
@@ -839,9 +939,6 @@ static int steam_sensors_register(struct steam_device *steam)
 	struct input_dev *sensors;
 	int ret;
 
-	if (!(steam->quirks & STEAM_QUIRK_DECK))
-		return 0;
-
 	rcu_read_lock();
 	sensors = rcu_dereference(steam->sensors);
 	rcu_read_unlock();
@@ -856,8 +953,14 @@ static int steam_sensors_register(struct steam_device *steam)
 
 	input_set_drvdata(sensors, steam);
 	sensors->dev.parent = &hdev->dev;
+	if (!(steam->quirks & STEAM_QUIRK_DECK)) {
+		sensors->open = steam_sensor_open;
+		sensors->close = steam_sensor_close;
+	}
 
-	sensors->name = "Steam Deck Motion Sensors";
+	sensors->name = steam->quirks & STEAM_QUIRK_DECK ?
+		"Steam Deck Motion Sensors" :
+		"Steam Controller Motion Sensors";
 	sensors->phys = hdev->phys;
 	sensors->uniq = steam->serial_no;
 	sensors->id.bustype = hdev->bus;
@@ -869,25 +972,34 @@ static int steam_sensors_register(struct steam_device *steam)
 	__set_bit(EV_MSC, sensors->evbit);
 	__set_bit(MSC_TIMESTAMP, sensors->mscbit);
 
-	input_set_abs_params(sensors, ABS_X, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_Y, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_Z, -STEAM_DECK_ACCEL_RANGE,
-			STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
-	input_abs_set_res(sensors, ABS_X, STEAM_DECK_ACCEL_RES_PER_G);
-	input_abs_set_res(sensors, ABS_Y, STEAM_DECK_ACCEL_RES_PER_G);
-	input_abs_set_res(sensors, ABS_Z, STEAM_DECK_ACCEL_RES_PER_G);
-
-	input_set_abs_params(sensors, ABS_RX, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_RY, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_set_abs_params(sensors, ABS_RZ, -STEAM_DECK_GYRO_RANGE,
-			STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
-	input_abs_set_res(sensors, ABS_RX, STEAM_DECK_GYRO_RES_PER_DPS);
-	input_abs_set_res(sensors, ABS_RY, STEAM_DECK_GYRO_RES_PER_DPS);
-	input_abs_set_res(sensors, ABS_RZ, STEAM_DECK_GYRO_RES_PER_DPS);
+	if (steam->quirks & STEAM_QUIRK_DECK) {
+		input_set_abs_params(sensors, ABS_X, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Y, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Z, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
+	} else {
+		input_set_abs_params(sensors, ABS_X, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Y, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+		input_set_abs_params(sensors, ABS_Z, -STEAM_ACCEL_RANGE,
+				STEAM_ACCEL_RANGE, STEAM_ACCEL_FUZZ, 0);
+	}
+	input_abs_set_res(sensors, ABS_X, STEAM_ACCEL_RES_PER_G);
+	input_abs_set_res(sensors, ABS_Y, STEAM_ACCEL_RES_PER_G);
+	input_abs_set_res(sensors, ABS_Z, STEAM_ACCEL_RES_PER_G);
+
+	input_set_abs_params(sensors, ABS_RX, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_set_abs_params(sensors, ABS_RY, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_set_abs_params(sensors, ABS_RZ, -STEAM_GYRO_RANGE,
+			STEAM_GYRO_RANGE, STEAM_GYRO_FUZZ, 0);
+	input_abs_set_res(sensors, ABS_RX, STEAM_GYRO_RES_PER_DPS);
+	input_abs_set_res(sensors, ABS_RY, STEAM_GYRO_RES_PER_DPS);
+	input_abs_set_res(sensors, ABS_RZ, STEAM_GYRO_RES_PER_DPS);
 
 	ret = input_register_device(sensors);
 	if (ret)
@@ -918,9 +1030,6 @@ static void steam_sensors_unregister(struct steam_device *steam)
 {
 	struct input_dev *sensors;
 
-	if (!(steam->quirks & STEAM_QUIRK_DECK))
-		return;
-
 	rcu_read_lock();
 	sensors = rcu_dereference(steam->sensors);
 	rcu_read_unlock();
@@ -968,6 +1077,12 @@ static int steam_register(struct steam_device *steam)
 			strscpy(steam->serial_no, "XXXXXXXXXX",
 					sizeof(steam->serial_no));
 
+		ret = steam_get_attributes(steam);
+		if (ret < 0)
+			hid_err(steam->hdev,
+				"%s:steam_get_attributes failed with error %d\n",
+				__func__, ret);
+
 		hid_info(steam->hdev, "Steam Controller '%s' connected",
 				steam->serial_no);
 
@@ -1246,6 +1361,10 @@ static int steam_probe(struct hid_device *hdev,
 	INIT_LIST_HEAD(&steam->list);
 	INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
 	steam->sensor_timestamp_us = 0;
+	if (steam->quirks & STEAM_QUIRK_DECK)
+		steam->sensor_update_rate_us = 4000;
+	else
+		steam->sensor_update_rate_us = 9000;
 	INIT_WORK(&steam->unregister_work, steam_work_unregister_cb);
 
 	/*
@@ -1491,6 +1610,16 @@ static const struct steam_axis_mapping steam_controller_axis_mappings[] = {
 	{ /* sentinel */ },
 };
 
+static const struct steam_axis_mapping steam_controller_imu_mappings[] = {
+	{ ABS_X,   1, 28 },
+	{ ABS_Z,  -1, 30 },
+	{ ABS_Y,   1, 32 },
+	{ ABS_RX,  1, 34 },
+	{ ABS_RZ,  1, 36 },
+	{ ABS_RY,  1, 38 },
+	{ /* sentinel */ },
+};
+
 static void steam_do_input_event(struct steam_device *steam,
 		struct input_dev *input, u8 *data)
 {
@@ -1533,6 +1662,17 @@ static void steam_do_input_event(struct steam_device *steam,
 	input_sync(input);
 }
 
+static void steam_do_sensors_event(struct steam_device *steam,
+		struct input_dev *sensors, u8 *data)
+{
+	steam->sensor_timestamp_us += steam->sensor_update_rate_us;
+
+	input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
+	steam_map_axes(sensors, steam_controller_imu_mappings, data);
+
+	input_sync(sensors);
+}
+
 /*
  * The size for this message payload is 56.
  * The known values are:
@@ -1727,14 +1867,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 static void steam_do_deck_sensors_event(struct steam_device *steam,
 		struct input_dev *sensors, u8 *data)
 {
-	/*
-	 * The deck input report is received every 4 ms on average,
-	 * with a jitter of +/- 4 ms even though the USB descriptor claims
-	 * that it uses 1 kHz.
-	 * Since the HID report does not include a sensor timestamp,
-	 * use a fixed increment here.
-	 */
-	steam->sensor_timestamp_us += 4000;
+	steam->sensor_timestamp_us += steam->sensor_update_rate_us;
 
 	if (!steam->gamepad_mode && lizard_mode)
 		return;
@@ -1819,6 +1952,9 @@ static int steam_raw_event(struct hid_device *hdev,
 		input = rcu_dereference(steam->input);
 		if (likely(input))
 			steam_do_input_event(steam, input, data);
+		sensors = rcu_dereference(steam->sensors);
+		if (likely(sensors))
+			steam_do_sensors_event(steam, sensors, data);
 		rcu_read_unlock();
 		break;
 	case ID_CONTROLLER_DECK_STATE:
-- 
2.54.0

Re: [PATCH v5 04/11] HID: steam: Add support for sensor events on the Steam Controller (2015)

[sashiko-bot]

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

Pre-existing issues:
- [High] Global list dangling pointer and Use-After-Free when steam_register fails during probe.
- [High] Race condition leading to double-free of input devices in workqueue callbacks.
--

--- Patch [4]: [PATCH v5 04/11] HID: steam: Add support for sensor events on the Steam Controller (2015) ---
commit c819d5d2af295f0bbbbb6b163562daebd8e1a93c
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Add support for sensor events on the Steam Controller (2015)

Sensor support was added for the Steam Deck previously, but Steam
Controller sensor events were never added. This adds that missing support,
bringing Steam Controller support much closer to feature parity with things
like SDL and Steam itself.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 983d18d1de4fe..1ceb044e170b4 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -918,9 +1030,6 @@ static void steam_sensors_unregister(struct steam_device *steam)
>  {
>  	struct input_dev *sensors;
>  
> -	if (!(steam->quirks & STEAM_QUIRK_DECK))
> -		return;
> -
>  	rcu_read_lock();
>  	sensors = rcu_dereference(steam->sensors);
>  	rcu_read_unlock();

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
leading to a double-free of the input devices in the workqueue callbacks?

If a device disconnects at the exact time a user opens the raw HID node,
steam->work_connect and steam->unregister_work can execute concurrently
on different CPUs. Both callbacks invoke steam_sensors_unregister() and
steam_input_unregister().

In steam_input_unregister(), the check-and-clear logic is not protected by a
mutex or lock:

drivers/hid/hid-steam.c:steam_input_unregister() {
	rcu_read_lock();
	input = rcu_dereference(steam->input);
	rcu_read_unlock();
	if (!input)
		return;
	RCU_INIT_POINTER(steam->input, NULL);
	synchronize_rcu();
	input_unregister_device(input);
}

Could both concurrent threads read a non-null pointer, drop the RCU lock,
and then both call input_unregister_device() on the same device?

[ ... ]

> @@ -968,6 +1077,12 @@ static int steam_register(struct steam_device *steam)
>  			strscpy(steam->serial_no, "XXXXXXXXXX",
>  					sizeof(steam->serial_no));
>  
> +		ret = steam_get_attributes(steam);
> +		if (ret < 0)
> +			hid_err(steam->hdev,
> +				"%s:steam_get_attributes failed with error %d\n",
> +				__func__, ret);
> +
>  		hid_info(steam->hdev, "Steam Controller '%s' connected",
>  				steam->serial_no);

[Severity: High]
This is a pre-existing issue, but could the error path in steam_register()
leave a dangling pointer in the global steam_devices list?

If steam_register() encounters an error during a wired device probe, such as
steam_input_register() failing to allocate memory, it returns an error code
but leaves the steam object on the global list:

drivers/hid/hid-steam.c:steam_register() {
...
steam_register_sensors_fail:
	steam_input_unregister(steam);
steam_register_input_fail:
	return ret;
}

The caller, steam_probe(), then jumps to the err_hw_close label, skipping
the err_steam_unregister label where the list state would be cleaned up:

drivers/hid/hid-steam.c:steam_probe() {
...
		ret = steam_register(steam);
		if (ret) {
			hid_err(hdev,
				"%s:steam_register failed with error %d\n",
				__func__, ret);
			goto err_hw_close;
		}
...
}

Because the steam object is managed by devm_kzalloc(), the kernel
automatically frees the memory when probe fails. A subsequent write to the
lizard_mode module parameter will trigger steam_param_set_lizard_mode(),
which iterates over the list and accesses the freed memory, leading to a
use-after-free.

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

[PATCH v5 05/11] HID: steam: Coalesce rumble packets

[Vicki Pfau]

The Steam Deck resets the haptic pattern every time it receives a rumble
packet, leading to weird discontinuities or sometimes cutting out entirely.
Instead of overloading the interface, Steam interally rate-limits sending
these packets, so we should too.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 1ceb044e170b..87b3817a2f69 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -343,6 +343,7 @@ struct steam_device {
 	bool did_mode_switch;
 	bool gamepad_mode;
 	struct work_struct rumble_work;
+	struct delayed_work coalesce_rumble_work;
 	u16 rumble_left;
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
@@ -603,10 +604,24 @@ static void steam_haptic_rumble_cb(struct work_struct *work)
 {
 	struct steam_device *steam = container_of(work, struct steam_device,
 							rumble_work);
+
 	steam_haptic_rumble(steam, 0, steam->rumble_left,
 		steam->rumble_right, 2, 0);
 }
 
+static void steam_coalesce_rumble_cb(struct work_struct *work)
+{
+	struct steam_device *steam = container_of(to_delayed_work(work),
+							struct steam_device,
+							coalesce_rumble_work);
+
+	steam_haptic_rumble(steam, 0, steam->rumble_left,
+		steam->rumble_right, 2, 0);
+
+	if (steam->rumble_left || steam->rumble_right)
+		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
+}
+
 #ifdef CONFIG_STEAM_FF
 static int steam_play_effect(struct input_dev *dev, void *data,
 				struct ff_effect *effect)
@@ -616,6 +631,14 @@ static int steam_play_effect(struct input_dev *dev, void *data,
 	steam->rumble_left = effect->u.rumble.strong_magnitude;
 	steam->rumble_right = effect->u.rumble.weak_magnitude;
 
+	/*
+	 * The interface gets somewhat overloaded when too many rumble
+	 * packets are sent in a row, so Steam throttles it to 20 Hz
+	 */
+	if (delayed_work_pending(&steam->coalesce_rumble_work))
+		return 0;
+
+	schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
 	return schedule_work(&steam->rumble_work);
 }
 #endif
@@ -1360,6 +1383,7 @@ static int steam_probe(struct hid_device *hdev,
 	INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb);
 	INIT_LIST_HEAD(&steam->list);
 	INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
+	INIT_DELAYED_WORK(&steam->coalesce_rumble_work, steam_coalesce_rumble_cb);
 	steam->sensor_timestamp_us = 0;
 	if (steam->quirks & STEAM_QUIRK_DECK)
 		steam->sensor_update_rate_us = 4000;
@@ -1426,6 +1450,7 @@ static int steam_probe(struct hid_device *hdev,
 	cancel_work_sync(&steam->work_connect);
 	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;
@@ -1444,6 +1469,7 @@ static void steam_remove(struct hid_device *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);
 	cancel_work_sync(&steam->unregister_work);
 	steam->client_hdev = NULL;
 	steam->client_opened = 0;
-- 
2.54.0

Re: [PATCH v5 05/11] HID: steam: Coalesce rumble packets

[sashiko-bot]

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

New issues:
- [High] Dynamic device unregistration fails to cancel `coalesce_rumble_work`, causing an infinite loop and resource leak.
- [Medium] Lockless `delayed_work_pending()` check bypasses rate limiting, causing haptic reset bugs.

Pre-existing issues:
- [High] Teardown order in `steam_remove()` causes Use-After-Free by cancelling work items before unregistering the input device.
--

--- Patch [5]: [PATCH v5 05/11] HID: steam: Coalesce rumble packets ---
commit d8418fccc5859b59417d02482c0b768383aa61ef
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Coalesce rumble packets

This commit introduces a 20Hz rate limit for rumble packets sent to the Steam
Deck to prevent the hardware from resetting haptic patterns on every packet.
It implements this rate limiting by using a delayed work queue to coalesce 
rumble updates.

> 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
[ ... ]
> @@ -603,10 +604,24 @@ static void steam_haptic_rumble_cb(struct work_struct *work)
>  {
>  	struct steam_device *steam = container_of(work, struct steam_device,
>  							rumble_work);
> +
>  	steam_haptic_rumble(steam, 0, steam->rumble_left,
>  		steam->rumble_right, 2, 0);
>  }
>  
> +static void steam_coalesce_rumble_cb(struct work_struct *work)
> +{
> +	struct steam_device *steam = container_of(to_delayed_work(work),
> +							struct steam_device,
> +							coalesce_rumble_work);
> +
> +	steam_haptic_rumble(steam, 0, steam->rumble_left,
> +		steam->rumble_right, 2, 0);
> +
> +	if (steam->rumble_left || steam->rumble_right)
> +		schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +}

[Severity: High]
Will this work item reschedule itself indefinitely if the device is dynamically
unregistered while a haptic effect is playing?

When the device is dynamically unregistered, such as during a wireless
disconnect or a mode switch, steam_unregister() destroys the input device
via steam_input_unregister().

However, steam_unregister() does not cancel coalesce_rumble_work, and the
input core does not proactively clear the rumble_left and rumble_right
magnitudes when an input device is destroyed. Because the input device is
gone, userspace can never send a 0-magnitude effect to stop the rumble,
leaving this worker to loop indefinitely every 50ms and leak CPU cycles
and USB bandwidth.

> +
>  #ifdef CONFIG_STEAM_FF
>  static int steam_play_effect(struct input_dev *dev, void *data,
>  				struct ff_effect *effect)
> @@ -616,6 +631,14 @@ static int steam_play_effect(struct input_dev *dev, void *data,
>  	steam->rumble_left = effect->u.rumble.strong_magnitude;
>  	steam->rumble_right = effect->u.rumble.weak_magnitude;
>  
> +	/*
> +	 * The interface gets somewhat overloaded when too many rumble
> +	 * packets are sent in a row, so Steam throttles it to 20 Hz
> +	 */
> +	if (delayed_work_pending(&steam->coalesce_rumble_work))
> +		return 0;
> +
> +	schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);
> +	return schedule_work(&steam->rumble_work);
>  }

[Severity: Medium]
Could this lockless check of delayed_work_pending() defeat the intended
rate limiting?

The workqueue core clears the pending bit immediately before executing the
worker callback. Because steam_coalesce_rumble_cb() acquires a mutex and
performs synchronous USB I/O, its execution time can be relatively large.

If a userspace process updates force feedback effects while the callback is
actively executing, delayed_work_pending() will evaluate to false. This
causes steam_play_effect() to immediately queue another rumble_work, which
then executes right after the callback finishes, sending two packets
back-to-back and bypassing the 50ms throttle.

[ ... ]
> @@ -1444,6 +1469,7 @@ static void steam_remove(struct hid_device *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);
>  	cancel_work_sync(&steam->unregister_work);
>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;

[Severity: High]
This is a pre-existing issue, but does the teardown order in steam_remove()
allow a use-after-free?

The works are cancelled before the input device is unregistered:

steam_remove() {
    ...
    cancel_work_sync(&steam->rumble_work);
    cancel_delayed_work_sync(&steam->coalesce_rumble_work);
    ...
    steam_unregister(steam);
}

If a userspace process continuously calls the EVIOCSFF ioctl to play
force-feedback effects while the device is being removed, the ioctl
path in steam_play_effect() can queue new rumble_work and
coalesce_rumble_work items after they have been cancelled.

Once steam_remove() completes, the devm-managed steam structure is freed,
but the newly queued work items will eventually execute and dereference
the freed memory.

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

[PATCH v5 06/11] HID: steam: Fully unregister controller when hidraw is opened

[Vicki Pfau]

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>
---
 drivers/hid/hid-steam.c | 73 +++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 46 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 87b3817a2f69..12203d61922f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -348,7 +348,6 @@ struct steam_device {
 	u16 rumble_right;
 	unsigned int sensor_timestamp_us;
 	unsigned int sensor_update_rate_us;
-	struct work_struct unregister_work;
 };
 
 static int steam_recv_report(struct steam_device *steam,
@@ -818,6 +817,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);
@@ -1077,6 +1077,7 @@ 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)
@@ -1084,6 +1085,7 @@ 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
@@ -1113,10 +1115,7 @@ 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;
 	}
 
 	spin_lock_irqsave(&steam->lock, flags);
@@ -1132,6 +1131,13 @@ static int steam_register(struct steam_device *steam)
 		if (ret != 0)
 			goto steam_register_sensors_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_sensors_fail:
@@ -1142,38 +1148,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);
 	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);
-		if (ret) {
+		if (ret)
 			hid_err(steam->hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-		}
 	} else {
 		steam_unregister(steam);
 	}
@@ -1207,31 +1216,6 @@ static void steam_mode_switch_cb(struct work_struct *work)
 	}
 }
 
-static void steam_work_unregister_cb(struct work_struct *work)
-{
-	struct steam_device *steam = container_of(work, struct steam_device,
-							unregister_work);
-	unsigned long flags;
-	bool connected;
-	bool opened;
-
-	spin_lock_irqsave(&steam->lock, flags);
-	opened = steam->client_opened;
-	connected = steam->connected;
-	spin_unlock_irqrestore(&steam->lock, flags);
-
-	if (connected) {
-		if (opened) {
-			steam_sensors_unregister(steam);
-			steam_input_unregister(steam);
-		} else {
-			steam_set_lizard_mode(steam, lizard_mode);
-			steam_input_register(steam);
-			steam_sensors_register(steam);
-		}
-	}
-}
-
 static bool steam_is_valve_interface(struct hid_device *hdev)
 {
 	struct hid_report_enum *rep_enum;
@@ -1277,7 +1261,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
 	steam->client_opened++;
 	spin_unlock_irqrestore(&steam->lock, flags);
 
-	schedule_work(&steam->unregister_work);
+	schedule_work(&steam->work_connect);
 
 	return 0;
 }
@@ -1292,7 +1276,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
 	steam->client_opened--;
 	spin_unlock_irqrestore(&steam->lock, flags);
 
-	schedule_work(&steam->unregister_work);
+	schedule_work(&steam->work_connect);
 }
 
 static int steam_client_ll_raw_request(struct hid_device *hdev,
@@ -1389,7 +1373,6 @@ static int steam_probe(struct hid_device *hdev,
 		steam->sensor_update_rate_us = 4000;
 	else
 		steam->sensor_update_rate_us = 9000;
-	INIT_WORK(&steam->unregister_work, steam_work_unregister_cb);
 
 	/*
 	 * With the real steam controller interface, do not connect hidraw.
@@ -1451,7 +1434,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;
 }
@@ -1470,7 +1452,6 @@ static void steam_remove(struct hid_device *hdev)
 	cancel_work_sync(&steam->work_connect);
 	cancel_work_sync(&steam->rumble_work);
 	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
-	cancel_work_sync(&steam->unregister_work);
 	steam->client_hdev = NULL;
 	steam->client_opened = 0;
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
-- 
2.54.0

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

[sashiko-bot]

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

New issues:
- [Medium] The local variable `do_add` in `steam_register()` can be evaluated uninitialized.

Pre-existing issues:
- [High] Teardown order in `steam_remove()` causes Use-After-Free and double-free vulnerabilities.
--

--- Patch [6]: [PATCH v5 06/11] HID: steam: Fully unregister controller when hidraw is opened ---
commit 14e48fa0937746f55f6b26048f8ecca7fe02bb3e
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Fully unregister controller when hidraw is opened

This patch changes the driver to fully unregister the controller
internally when hidraw is opened, leaving only the hidraw active until
it is closed. It also unifies the unregister and connect callbacks to
share identical logic.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 87b3817a2f69d..12203d61922f7 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -1084,6 +1085,7 @@ 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
> @@ -1113,10 +1115,7 @@ 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;
>  	}
>  
>  	spin_lock_irqsave(&steam->lock, flags);
> @@ -1132,6 +1131,13 @@ static int steam_register(struct steam_device *steam)
>  		if (ret != 0)
>  			goto steam_register_sensors_fail;
>  	}
> +
> +	if (do_add) {

[Severity: Medium]
Is it possible for do_add to be evaluated uninitialized here?

When steam_register() is invoked on an already-registered device,
steam->serial_no[0] is already populated. This causes the initialization block
for "if (!steam->serial_no[0])" to be skipped, meaning do_add is never
assigned a value. Could this undefined behavior lead to compiler
optimizations altering the control flow or causing redundant lock
acquisitions?

> +		mutex_lock(&steam_devices_lock);
> +		if (list_empty(&steam->list))
> +			list_add(&steam->list, &steam_devices);
> +		mutex_unlock(&steam_devices_lock);
> +	}
>  	return 0;

[ ... ]

> @@ -1470,7 +1452,6 @@ static void steam_remove(struct hid_device *hdev)
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_work_sync(&steam->rumble_work);
>  	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
> -	cancel_work_sync(&steam->unregister_work);

[Severity: High]
This is a pre-existing issue, but does the teardown order in steam_remove()
cause a Use-After-Free and double-free vulnerability?

Currently, work items like steam->work_connect are canceled before calling
hid_hw_stop(). Since the HID event stream is still active, an incoming
interrupt (such as a connection status report) can trigger steam_raw_event():

steam_raw_event()
  steam_do_connect_event()
    schedule_work(&steam->work_connect)

When steam_remove() completes, devres frees the steam_device struct, leaving
a dangling work item in the queue that triggers a UAF when executed.

Furthermore, if the rescheduled work executes concurrently with the end of
steam_remove(), both threads can invoke steam_unregister() simultaneously.
Because steam_unregister() lacks mutual exclusion, does this lead to
concurrent double-frees of the input and sensor devices?

>  	steam->client_hdev = NULL;
>  	steam->client_opened = 0;
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {

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

[PATCH v5 07/11] HID: steam: Rearrange teardown sequence

[Vicki Pfau]

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>
---
 drivers/hid/hid-steam.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 12203d61922f..663fda8a86fd 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1156,6 +1156,9 @@ static void steam_unregister(struct steam_device *steam)
 	steam_battery_unregister(steam);
 	steam_sensors_unregister(steam);
 	steam_input_unregister(steam);
+	cancel_work_sync(&steam->rumble_work);
+	cancel_delayed_work_sync(&steam->mode_switch);
+	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
 	mutex_lock(&steam_devices_lock);
 	list_del_init(&steam->list);
 	mutex_unlock(&steam_devices_lock);
@@ -1441,25 +1444,24 @@ 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);
+	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.54.0

Re: [PATCH v5 07/11] HID: steam: Rearrange teardown sequence

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free due to `work_connect` rescheduling after cancellation in `steam_remove`.
- [High] Race condition leading to use-after-free in `steam_probe` error path.
- [High] Lockless TOCTOU race in `steam_raw_event` leads to Use-After-Free of `steam->client_hdev`.
--

--- Patch [7]: [PATCH v5 07/11] HID: steam: Rearrange teardown sequence ---
commit 0f9077495b7412b91d8e827f54f32dc26780b709
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Rearrange teardown sequence

This patch reorders the teardown sequence for the Steam controller driver to
address a narrow window where callbacks could be scheduled with a dangling
pointer. It also moves the cancellation of rumble and mode switch work to
steam_unregister().

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 12203d61922f7..663fda8a86fdb 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -1441,25 +1444,24 @@ 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;

[Severity: High]
This is a pre-existing issue, but does this sequence leave a window for a
use-after-free on steam->client_hdev?

In steam_remove(), hid_destroy_device(steam->client_hdev) is called to free
the virtual client device while the hardware is still active, since
hid_hw_stop() has not been called yet.

Meanwhile, steam_raw_event() locklessly checks if the client is opened before
dereferencing steam->client_hdev:

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

If an interrupt fires and passes the client_opened check, could it get
preempted, and then resume to call hid_input_report() on the freed
client_hdev pointer after steam_remove() completes the destruction?

> +	spin_unlock_irqrestore(&steam->lock, flags);
> +	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);
>  }

[Severity: High]
This is a pre-existing issue, but can steam->work_connect still be
rescheduled after it is cancelled here?

In steam_remove(), cancel_work_sync(&steam->work_connect) is called, but
hid_hw_stop(hdev) isn't called until the very end.

If a wireless connect or disconnect event arrives in between,
steam_do_connect_event() unconditionally reschedules the work:

drivers/hid/hid-steam.c:steam_do_connect_event() {
    ...
    if (changed && schedule_work(&steam->work_connect) == 0)
        ...
}

When steam_remove() finishes, the steam object is freed by devres, leaving a
dangling work item in the system workqueue. Will this lead to a
use-after-free when the scheduled work executes?

[Severity: High]
This is another pre-existing issue, but does the error path in steam_probe()
have a similar race condition with work_connect?

In the steam_probe() error path, steam_unregister() is called before
cancel_work_sync(&steam->work_connect):

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

steam_unregister() clears the device state by setting steam->serial_no[0] = 0.

If steam_work_connect_cb() is currently queued and runs concurrently, it
will see connected == true and call steam_register().

Because serial_no was cleared, steam_register() re-registers all input
devices and adds the steam object back to the global steam_devices list.

After the work finishes, cancel_work_sync() silently returns, steam_probe()
returns an error, and the steam object is freed by devres. Does this leave a
freed object in the global list and registered to input subsystems, leading
to a use-after-free?

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

[PATCH v5 08/11] HID: steam: Improve logging and other cleanup

[Vicki Pfau]

Adds more logging as appropriate, reindents an enum to match surrounding
style, as well as cleaning up some places where we can use guard() instead
of doing locking and unlocking manually.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 56 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 663fda8a86fd..222b5751040a 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -246,14 +246,14 @@ enum {
 /* Input report identifiers */
 enum
 {
-	ID_CONTROLLER_STATE = 1,
-	ID_CONTROLLER_DEBUG = 2,
-	ID_CONTROLLER_WIRELESS = 3,
-	ID_CONTROLLER_STATUS = 4,
-	ID_CONTROLLER_DEBUG2 = 5,
-	ID_CONTROLLER_SECONDARY_STATE = 6,
-	ID_CONTROLLER_BLE_STATE = 7,
-	ID_CONTROLLER_DECK_STATE = 9
+	ID_CONTROLLER_STATE		= 1,
+	ID_CONTROLLER_DEBUG		= 2,
+	ID_CONTROLLER_WIRELESS		= 3,
+	ID_CONTROLLER_STATUS		= 4,
+	ID_CONTROLLER_DEBUG2		= 5,
+	ID_CONTROLLER_SECONDARY_STATE	= 6,
+	ID_CONTROLLER_BLE_STATE		= 7,
+	ID_CONTROLLER_DECK_STATE	= 9,
 };
 
 /* Read-only attributes */
@@ -379,9 +379,16 @@ static int steam_recv_report(struct steam_device *steam,
 	ret = hid_hw_raw_request(steam->hdev, 0x00,
 			buf, hid_report_len(r) + 1,
 			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
-	if (ret > 0)
-		memcpy(data, buf + 1, min(size, ret - 1));
+	if (ret > 0) {
+		ret = min(size, ret - 1);
+		memcpy(data, buf + 1, ret);
+	}
 	kfree(buf);
+
+	if (ret < 0)
+		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
+	else
+		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
 	return ret;
 }
 
@@ -409,6 +416,8 @@ static int steam_send_report(struct steam_device *steam,
 	/* The report ID is always 0 */
 	memcpy(buf + 1, cmd, size);
 
+	hid_dbg(steam->hdev, "Sending report %*ph\n", size, cmd);
+
 	/*
 	 * Sometimes the wireless controller fails with EPIPE
 	 * when sending a feature report.
@@ -481,22 +490,21 @@ static int steam_get_serial(struct steam_device *steam)
 	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
 	u8 reply[3 + STEAM_SERIAL_LEN + 1];
 
-	mutex_lock(&steam->report_mutex);
+	guard(mutex)(&steam->report_mutex);
 	ret = steam_send_report(steam, cmd, sizeof(cmd));
 	if (ret < 0)
-		goto out;
+		return ret;
 	ret = steam_recv_report(steam, reply, sizeof(reply));
 	if (ret < 0)
-		goto out;
+		return ret;
 	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
 	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
-		ret = -EIO;
-		goto out;
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				(int)sizeof(reply), reply);
+		return -EIO;
 	}
 	reply[3 + STEAM_SERIAL_LEN] = 0;
 	strscpy(steam->serial_no, reply + 3, reply[1]);
-out:
-	mutex_unlock(&steam->report_mutex);
 	return ret;
 }
 
@@ -516,8 +524,11 @@ static int steam_get_attributes(struct steam_device *steam)
 	ret = steam_recv_report(steam, reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2)
+	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2) {
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				(int)sizeof(reply), reply);
 		return -EIO;
+	}
 
 	size = min(reply[1], sizeof(reply) - 2);
 	for (i = 0; i + sizeof(*attr) <= size; i += sizeof(*attr)) {
@@ -539,11 +550,8 @@ static int steam_get_attributes(struct steam_device *steam)
  */
 static inline int steam_request_conn_status(struct steam_device *steam)
 {
-	int ret;
-	mutex_lock(&steam->report_mutex);
-	ret = steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE);
-	mutex_unlock(&steam->report_mutex);
-	return ret;
+	guard(mutex)(&steam->report_mutex);
+	return steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE);
 }
 
 /*
@@ -1201,6 +1209,7 @@ static void steam_mode_switch_cb(struct work_struct *work)
 		return;
 
 	steam->gamepad_mode = !steam->gamepad_mode;
+	hid_dbg(steam->hdev, "%s: switching gamepad mode to %i\n", __func__, steam->gamepad_mode);
 	if (steam->gamepad_mode)
 		steam_set_lizard_mode(steam, false);
 	else {
@@ -1841,6 +1850,7 @@ static void steam_do_deck_input_event(struct steam_device *steam,
 		steam->did_mode_switch = false;
 		cancel_delayed_work(&steam->mode_switch);
 	} else if (!steam->client_opened && start_pressed && !steam->did_mode_switch) {
+		hid_dbg(steam->hdev, "%s: doing mode switch\n", __func__);
 		steam->did_mode_switch = true;
 		schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100);
 	}
-- 
2.54.0

Re: [PATCH v5 08/11] HID: steam: Improve logging and other cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added error logging in `steam_get_serial` leaks uninitialized kernel stack memory to the kernel log via `hid_err`.
--

--- Patch [8]: [PATCH v5 08/11] HID: steam: Improve logging and other cleanup ---
commit 15f36444bb8b36e80ca21a45d7b255628f820523
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Improve logging and other cleanup

This patch adds additional logging to the hid-steam driver and aligns
enumeration definitions. It also cleans up several error paths by replacing
manual mutex lock and unlock calls with scope-based guard() protection.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 663fda8a86fdb..222b5751040a6 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

> @@ -481,22 +490,21 @@ static int steam_get_serial(struct steam_device *steam)
>  	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
>  	u8 reply[3 + STEAM_SERIAL_LEN + 1];
>  
> -	mutex_lock(&steam->report_mutex);
> +	guard(mutex)(&steam->report_mutex);
>  	ret = steam_send_report(steam, cmd, sizeof(cmd));
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  	ret = steam_recv_report(steam, reply, sizeof(reply));
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
>  	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
> -		ret = -EIO;
> -		goto out;
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				(int)sizeof(reply), reply);

[Severity: High]
Could this leak uninitialized stack memory to the kernel log?

The reply array is allocated on the stack at the start of steam_get_serial()
without being zero-initialized.

If steam_recv_report() reads a short or invalid response, it may return
fewer bytes than the full size of the buffer. By unconditionally printing
sizeof(reply) bytes in this error path, it looks like uninitialized kernel
stack data will be dumped to dmesg.

Would it be safer to print only the number of bytes actually received (ret),
or to zero-initialize the reply array when it is declared?

> +		return -EIO;
>  	}
>  	reply[3 + STEAM_SERIAL_LEN] = 0;
>  	strscpy(steam->serial_no, reply + 3, reply[1]);
> -out:
> -	mutex_unlock(&steam->report_mutex);
>  	return ret;
>  }
>  

[ ... ]

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

[PATCH v5 09/11] HID: steam: Zero-initialize reply in serial lookup

[Vicki Pfau]

When requesting the serial number from a controller, the function will do
some basic bounds checking to make sure the reply is valid, as well as
capping off the reply with a null byte before copying. However, the error
logging can leak uninitialized memory in some cases. We can simplify and
solve this by just zero-initalizing the reply memory eagerly instead.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 222b5751040a..ddd439dd069b 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -488,7 +488,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};
 
 	guard(mutex)(&steam->report_mutex);
 	ret = steam_send_report(steam, cmd, sizeof(cmd));
@@ -503,7 +503,6 @@ static int steam_get_serial(struct steam_device *steam)
 				(int)sizeof(reply), reply);
 		return -EIO;
 	}
-	reply[3 + STEAM_SERIAL_LEN] = 0;
 	strscpy(steam->serial_no, reply + 3, reply[1]);
 	return ret;
 }
-- 
2.54.0

[PATCH v5 10/11] HID: steam: Reject short reads

[Vicki Pfau]

Steam Controller FEATURE reports encode the size of the message in the
message itself. Previously we were trusting that the size reported matched
the size we actually read, leading to a potential issue with short reads.
Instead, we should actually verify the length of the read.

Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index ddd439dd069b..3b4a588c20ad 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -357,6 +357,13 @@ static int steam_recv_report(struct steam_device *steam,
 	u8 *buf;
 	int ret;
 
+	/*
+	 * All reports start with a two byte header.
+	 * We must read at least two bytes to get a sensible output.
+	 */
+	if (size < 2)
+		return -EINVAL;
+
 	r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
 	if (!r) {
 		hid_err(steam->hdev, "No HID_FEATURE_REPORT submitted -  nothing to read\n");
@@ -380,16 +387,30 @@ static int steam_recv_report(struct steam_device *steam,
 			buf, hid_report_len(r) + 1,
 			HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 	if (ret > 0) {
-		ret = min(size, ret - 1);
-		memcpy(data, buf + 1, ret);
+		/* Remove the report ID from the return buffer */
+		ret--;
+		size = min(size, ret);
+		memcpy(data, buf + 1, size);
 	}
 	kfree(buf);
 
 	if (ret < 0)
 		hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
 	else
-		hid_dbg(steam->hdev, "Received report %*ph\n", ret, data);
-	return ret;
+		hid_dbg(steam->hdev, "Received report %*ph\n", size, data);
+	if (ret < 0)
+		return ret;
+
+	if (ret < 2) {
+		hid_err(steam->hdev, "%s: reply too short\n", __func__);
+		return -EPROTO;
+	}
+	if (ret < data[1] + 2) {
+		hid_err(steam->hdev, "%s: expected %u bytes, read %i\n",
+				__func__, data[1] + 2, ret);
+		return -EPROTO;
+	}
+	return size;
 }
 
 static int steam_send_report(struct steam_device *steam,
-- 
2.54.0

[PATCH v5 11/11] HID: steam: Retry send/recv reports if stale

[Vicki Pfau]

Sometimes recv report will reply with a stale result from a previous send
report. Instead of failing out, we should retry them, as they generally
reply correctly after three tries, give or take.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/hid/hid-steam.c | 54 +++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 3b4a588c20ad..6199f67f3c4c 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -501,6 +501,43 @@ static int steam_write_settings(struct steam_device *steam,
 	return steam_recv_report(steam, cmd, 2 + cmd[1]);
 }
 
+static int steam_exchange_report(struct steam_device *steam, u8 *cmd, int csize,
+		u8 *reply, int rsize)
+{
+	unsigned int retries = 5;
+	int ret;
+
+	guard(mutex)(&steam->report_mutex);
+	do {
+		ret = steam_send_report(steam, cmd, csize);
+		if (ret < 0)
+			return ret;
+		ret = steam_recv_report(steam, reply, rsize);
+		/*
+		 * Sometimes this can fail on the first few tries on the Steam
+		 * Controller (2015). It appears to be a firmware bug, and Steam
+		 * itself just retries, so we should also retry a few times to
+		 * see if we get it.
+		 */
+		if (ret == -EPROTO)
+			continue;
+		if (ret < 0) {
+			hid_err(steam->hdev, "%s: error reading reply (%*ph)\n",
+					__func__, csize, cmd);
+			return ret;
+		}
+		if (reply[0] == cmd[0] && reply[1] >= 1)
+			break;
+		if (retries > 0)
+			continue;
+		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
+				rsize, reply);
+		return -EPROTO;
+	} while (retries--);
+
+	return ret;
+}
+
 static int steam_get_serial(struct steam_device *steam)
 {
 	/*
@@ -511,15 +548,10 @@ static int steam_get_serial(struct steam_device *steam)
 	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
 	u8 reply[3 + STEAM_SERIAL_LEN + 1] = {0};
 
-	guard(mutex)(&steam->report_mutex);
-	ret = steam_send_report(steam, cmd, sizeof(cmd));
+	ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	ret = steam_recv_report(steam, reply, sizeof(reply));
-	if (ret < 0)
-		return ret;
-	if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 ||
-	    reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
+	if (reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) {
 		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
 				(int)sizeof(reply), reply);
 		return -EIO;
@@ -537,14 +569,10 @@ static int steam_get_attributes(struct steam_device *steam)
 	int i;
 	struct steam_controller_attribute *attr;
 
-	guard(mutex)(&steam->report_mutex);
-	ret = steam_send_report(steam, cmd, sizeof(cmd));
-	if (ret < 0)
-		return ret;
-	ret = steam_recv_report(steam, reply, sizeof(reply));
+	ret = steam_exchange_report(steam, cmd, sizeof(cmd), reply, sizeof(reply));
 	if (ret < 0)
 		return ret;
-	if (reply[0] != ID_GET_ATTRIBUTES_VALUES || reply[1] < 2) {
+	if (reply[1] < 2) {
 		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
 				(int)sizeof(reply), reply);
 		return -EIO;
-- 
2.54.0