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

[PATCH v3 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.

This is a minor interation on v2 that fixes a few issues that came up
during review of v2.

Vicki Pfau (10):
  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 sequences
  HID: steam: Improve logging and other cleanup
  HID: steam: Reject short reads
  HID: steam: Retry send/recv reports if stale

 MAINTAINERS             |   6 +
 drivers/hid/hid-steam.c | 720 +++++++++++++++++++++++++++-------------
 2 files changed, 490 insertions(+), 236 deletions(-)

-- 
2.54.0

[PATCH v3 01/10] 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 4ecd282f8f52..2ed29e970023 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11500,6 +11500,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 v3 02/10] 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..1b0367c98eac 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((__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 v3 02/10] HID: steam: Refactor and clean up report parsing

[sashiko-bot]

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

New issues:
- [Low] Redundant pointer cast discards 'const' qualifier.

Pre-existing issues:
- [High] The teardown sequence in `steam_remove()` cancels workqueues before stopping hardware and input devices, creating a race condition that allows works to be re-queued, leading to a use-after-free.
- [High] Probe error paths skip unregistering the device, leaking resources and corrupting the global `steam_devices` list.
--

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

HID: steam: Refactor and clean up report parsing

This commit refactors the parsing of buttons and axes into a unified
array-based mapping approach rather than parsing each out individually.
It introduces mapping arrays and helper functions to simplify the event
handling code.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a854d6360a0e3..1b0367c98eac1 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -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((__le16 *)data);

[Severity: Low]
Since the data parameter is now const, and get_unaligned_le16() natively
accepts a const void pointer, does the explicit cast to (__le16 *)
unnecessarily discard the const qualifier and risk a -Wcast-qual warning?

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at the teardown sequence in
steam_remove(), background tasks are canceled before unregistering the
device and stopping the hardware:

drivers/hid/hid-steam.c:steam_remove() {
        ...
        cancel_work_sync(&steam->rumble_work);
        cancel_delayed_work_sync(&steam->mode_switch);
        cancel_delayed_work_sync(&steam->coalesce_rumble_work);
        hid_hw_stop(hdev);
}

Is it possible for a use-after-free to occur if an incoming hardware event
triggers steam_do_connect_event() and dynamically re-queues the work items
during this race window?

[Severity: High]
This is also a pre-existing issue, but looking at the error paths in
steam_probe(), could we be leaking resources and corrupting the global
steam_devices list?

If steam_register() adds the device to the list but subsequently fails
(for example, during steam_input_register()), it returns an error while
leaving the struct attached:

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

The err_hw_stop label completely bypasses err_steam_unregister, leaving a
freed pointer in the steam_devices list which could cause a kernel crash
when iterating the list later.

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

[PATCH v3 03/10] 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 1b0367c98eac..add9f4f27fff 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 v3 04/10] 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 add9f4f27fff..6029c055a6cf 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 v3 04/10] HID: steam: Add support for sensor events on the Steam Controller (2015)

[sashiko-bot]

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

New issues:
- [Medium] The newly added `sensors` IMU configuration is silently destroyed when userspace closes the concurrent `input` evdev node or briefly toggles the hidraw client.
- [Medium] The IMU gyroscope Y-axis mapping for the Steam Controller has an inverted sign, breaking the right-handed coordinate system.

Pre-existing issues:
- [High] Use-After-Free when `steam_register()` fails during `steam_probe()`.
- [High] Concurrent execution of connection and unregistration work queues causes double-registration and permanent memory leaks of input devices.
- [High] Information Leak: `steam_send_report()` sends uninitialized kernel heap memory to the USB device.
- [High] HID I/O requests are issued after the hardware transport is torn down, violating hardware lifecycle rules.
--

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

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 add9f4f27fff9..6029c055a6cf3 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -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));

[Severity: High]
This is a pre-existing issue, but does steam_send_report() send uninitialized
kernel heap memory to the USB device?

Looking at steam_send_report(), it allocates a buffer using
hid_alloc_report_buf(), which internally calls kmalloc() instead of
kzalloc(). It then copies the payload and sends max(size, 64) + 1 bytes via
hid_hw_raw_request().

When steam_get_attributes() calls this with a 2-byte payload, does this
result in up to 63 bytes of uninitialized kernel memory being transmitted to
the USB device?

> +	if (ret < 0)
> +		return ret;
[ ... ]
> @@ -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);

[Severity: Medium]
Is the newly added sensors IMU configuration silently destroyed when userspace
closes the concurrent input evdev node?

If userspace has both the sensors node and the input node open, and then
closes the input node, steam_input_close() executes and evaluates
!steam->client_opened && lizard_mode. This calls
steam_set_lizard_mode(steam, true), which sends ID_LOAD_DEFAULT_SETTINGS.

Since ID_LOAD_DEFAULT_SETTINGS entirely resets the controller's hardware
configuration and turns off the IMU, does the sensors node remain open while
the physical IMU silently stops reporting data? Could this same state clobbering
occur if the raw HID client is quickly opened and closed?

> +
> +	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);

[Severity: High]
This is a pre-existing issue, but does calling steam_write_settings() here
violate hardware lifecycle rules during device removal?

In steam_remove(), the driver calls hid_hw_close(hdev) and
hid_hw_stop(hdev), which tears down the USB transport layer. Following this,
steam_unregister() invokes steam_sensors_unregister(), which calls
input_unregister_device().

If a userspace application holds the evdev node open, this triggers the
steam_sensor_close() callback. Since steam_write_settings() issues I/O
requests to a stopped HID device, can this access torn-down transport structures
and cause use-after-free conditions or kernel panics during device unplug?

> +}
[ ... ]
> @@ -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();
>  	if (sensors) {
>  		dbg_hid("%s: already connected\n", __func__);
>  		return 0;
>  	}
>  
>  	sensors = input_allocate_device();

[Severity: High]
This is a pre-existing issue, but can concurrent execution of the connection and
unregistration work queues cause double-registration and permanent memory leaks?

steam_work_connect_cb() and steam_work_unregister_cb() run on different
workqueues. If they execute concurrently, they can both read steam->sensors
as NULL here because the check only holds rcu_read_lock() and provides no
mutual exclusion.

Could both threads allocate a new device, call input_register_device(), and
publish their pointer via rcu_assign_pointer(), leaving the first device
permanently orphaned and leaked?

>  	if (!sensors)
[ ... ]
> @@ -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 does a failure in steam_register() lead to
a Use-After-Free during steam_probe()?

Early in steam_register(), the steam object is added to the global
steam_devices list. If steam_register() fails, it returns the error code
without removing steam from the list.

Back in steam_probe(), the error is caught, and the code jumps to
goto err_hw_close;, bypassing the err_steam_unregister: label.
steam_probe() exits with an error, and the driver core automatically frees
the steam object allocated with devm_kzalloc().

Does the freed object remain in the global steam_devices list, where any
subsequent read or write to the lizard_mode sysfs parameter would dereference
the freed pointer?

>  
[ ... ]
> @@ -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 },

[Severity: Medium]
Does the ABS_RZ mapping have an inverted sign here, breaking the right-handed
coordinate system?

The Accel Y-axis is mapped to ABS_Z with a sign of -1, which matches the
Steam Deck mapping, indicating the physical IMU orientation is identical. On the
Steam Deck, the Gyro Y-axis is mapped to ABS_RZ with a sign of -1 to
maintain the correct coordinate system.

Since the Steam Controller mapping uses 1 for Gyro Y (ABS_RZ) instead of
-1, will this report mathematically inverted yaw rotation to applications?

> +	{ ABS_RY,  1, 38 },
> +	{ /* sentinel */ },
> +};
> +

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

[PATCH v3 05/10] 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 6029c055a6cf..553748813901 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 v3 05/10] HID: steam: Coalesce rumble packets

[sashiko-bot]

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

New issues:
- [Medium] Unconditional self-rescheduling causes severe haptic stutter by spamming redundant packets at 20Hz.

Pre-existing issues:
- [High] Use-after-free during device removal when a force-feedback effect is active.
--

--- Patch [5]: [PATCH v3 05/10] HID: steam: Coalesce rumble packets ---
commit 8356fb642b10f58402cd235e2f8093f10a60cd43
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Coalesce rumble packets

The Steam Deck resets the haptic pattern every time it receives a rumble
packet. This patch rate-limits sending these packets to 20Hz to prevent
discontinuities and cutting out.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 6029c055a6cf3..5537488139011 100644
> --- 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: Medium]
Will this unconditionally self-reschedule and send identical packets at 20Hz
as long as a continuous force-feedback effect is active?

If a constant rumble effect is playing, this worker will continuously send
hardware reports and reschedule itself at 20Hz even if the magnitude has not
changed.

Since the commit message notes that the Steam Deck resets its haptic pattern
upon receiving a rumble packet, won't spamming these redundant packets force
continuous resets and cause the exact haptic stutter this patch is trying to
fix?

Could we add a check to only send packets and reschedule if the requested
rumble magnitudes have actually changed?

[ ... ]

> @@ -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 canceling these work items before
unregistering the input device cause a use-after-free on device removal?

If the device is physically unplugged while a force-feedback effect is
playing, the removal path cancels the rumble works and then proceeds to call
steam_unregister().

When the input device is unregistered, the input core flushes and stops all
active effects. This operation routes through to steam_play_effect() with a
zero magnitude.

Since steam_play_effect() unconditionally re-arms the work items by calling
schedule_work() and schedule_delayed_work(), the work items we just cancelled
will be queued again. When device removal completes, the steam_device
structure memory is freed by devres.

The re-armed works will execute later and dereference the freed memory. Should
the work items be cancelled after steam_unregister() instead to prevent this?

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

[PATCH v3 06/10] 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 | 60 +++++++++++++----------------------------
 1 file changed, 18 insertions(+), 42 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 553748813901..0771ad4f5f2c 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)
@@ -1142,38 +1143,41 @@ static int steam_register(struct steam_device *steam)
 
 static void steam_unregister(struct steam_device *steam)
 {
+	if (!steam->serial_no[0])
+		return;
+
+	hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+			steam->serial_no);
 	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 +1211,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 +1256,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 +1271,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 +1368,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 +1429,6 @@ static int steam_probe(struct hid_device *hdev,
 	cancel_delayed_work_sync(&steam->mode_switch);
 	cancel_work_sync(&steam->rumble_work);
 	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
-	cancel_work_sync(&steam->unregister_work);
 
 	return ret;
 }
@@ -1470,7 +1447,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 v3 06/10] 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:

Pre-existing issues:
- [High] If `steam_register()` fails during `steam_probe()`, the device is left in the global `steam_devices` list, leading to a Use-After-Free.
- [High] In `steam_remove()`, workqueues are cancelled before stopping the HID hardware and unregistering input devices, allowing works to be re-queued and executed after the device is freed.
--

--- Patch [6]: [PATCH v3 06/10] HID: steam: Fully unregister controller when hidraw is opened ---
commit b7eebc07a61af5a54e2737e2f58726a8f773d095
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Fully unregister controller when hidraw is opened

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

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

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 5537488139011..0771ad4f5f2c2 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a failed
probe to leak the device pointer in the global steam_devices list?

If steam_register() fails during a wired controller's steam_probe(), the
failure causes steam_probe() to jump to an error label that bypasses
err_steam_unregister, which would have cleanly removed it via
steam_unregister().

The devres subsystem then frees the steam structure. Any subsequent iteration
of steam_devices—such as modifying the lizard_mode module parameter via sysfs
or plugging in another controller—will dereference this freed memory.

> @@ -1470,7 +1447,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;

[Severity: High]
This is a pre-existing issue, but does cancelling the workqueues here create
a race condition?

In steam_remove(), workqueues (like rumble_work and work_connect) are
cancelled early in the teardown sequence. However, hid_hw_stop() is called
after these cancellations, meaning the HID transport is still active and
incoming reports can trigger steam_do_connect_event(), which re-queues
work_connect.

Furthermore, steam_unregister() is called at the very end of steam_remove().
Because the input device is still active, userspace can send force-feedback
events that call steam_play_effect(), re-queuing rumble_work and
coalesce_rumble_work.

Since these works are re-queued after the cancel_work_sync() calls, could
they execute after the steam structure is freed by devres, causing a
Use-After-Free?

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

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

[Vicki Pfau]

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

Signed-off-by: Vicki Pfau <vi@endrift.com>

HID: steam: Reorder probe
---
 drivers/hid/hid-steam.c | 67 ++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 0771ad4f5f2c..9dc77cd76edb 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1328,6 +1328,7 @@ static int steam_probe(struct hid_device *hdev,
 {
 	struct steam_device *steam;
 	int ret;
+	unsigned long flags;
 
 	ret = hid_parse(hdev);
 	if (ret) {
@@ -1353,6 +1354,14 @@ static int steam_probe(struct hid_device *hdev,
 	if (!steam)
 		return -ENOMEM;
 
+	/*
+	 * With the real steam controller interface, do not connect hidraw.
+	 * Instead, create the client_hid and connect that.
+	 */
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
+	if (ret)
+		return ret;
+
 	steam->hdev = hdev;
 	hid_set_drvdata(hdev, steam);
 	spin_lock_init(&steam->lock);
@@ -1369,22 +1378,6 @@ static int steam_probe(struct hid_device *hdev,
 	else
 		steam->sensor_update_rate_us = 9000;
 
-	/*
-	 * 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 err_cancel_work;
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev,
-			"%s:hid_hw_open\n",
-			__func__);
-		goto err_hw_stop;
-	}
-
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver connected");
 		/* If using a wireless adaptor ask for connection status */
@@ -1398,14 +1391,22 @@ static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto err_hw_close;
+			goto err_hw_stop;
 		}
 	}
 
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev,
+			"%s:hid_hw_open\n",
+			__func__);
+		goto err_steam_unregister;
+	}
+
 	steam->client_hdev = steam_create_client_hid(hdev);
 	if (IS_ERR(steam->client_hdev)) {
 		ret = PTR_ERR(steam->client_hdev);
-		goto err_steam_unregister;
+		goto err_hw_close;
 	}
 	steam->client_hdev->driver_data = steam;
 
@@ -1417,18 +1418,20 @@ static int steam_probe(struct hid_device *hdev,
 
 err_destroy:
 	hid_destroy_device(steam->client_hdev);
-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:
+err_steam_unregister:
+	spin_lock_irqsave(&steam->lock, flags);
+	steam->client_opened = 0;
+	spin_unlock_irqrestore(&steam->lock, flags);
 	cancel_work_sync(&steam->work_connect);
+	if (steam->connected)
+		steam_unregister(steam);
 	cancel_delayed_work_sync(&steam->mode_switch);
+err_hw_stop:
 	cancel_work_sync(&steam->rumble_work);
 	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
+	hid_hw_stop(hdev);
 
 	return ret;
 }
@@ -1436,25 +1439,27 @@ static int steam_probe(struct hid_device *hdev,
 static void steam_remove(struct hid_device *hdev)
 {
 	struct steam_device *steam = hid_get_drvdata(hdev);
+	unsigned long flags;
 
 	if (!steam || hdev->group == HID_GROUP_STEAM) {
 		hid_hw_stop(hdev);
 		return;
 	}
 
+	hid_hw_close(hdev);
 	hid_destroy_device(steam->client_hdev);
-	cancel_delayed_work_sync(&steam->mode_switch);
-	cancel_work_sync(&steam->work_connect);
-	cancel_work_sync(&steam->rumble_work);
-	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
-	steam->client_hdev = NULL;
+	spin_lock_irqsave(&steam->lock, flags);
 	steam->client_opened = 0;
+	spin_unlock_irqrestore(&steam->lock, flags);
+	cancel_work_sync(&steam->work_connect);
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver disconnected");
 	}
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 	steam_unregister(steam);
+	cancel_work_sync(&steam->rumble_work);
+	cancel_delayed_work_sync(&steam->mode_switch);
+	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
+	hid_hw_stop(hdev);
 }
 
 static void steam_do_connect_event(struct steam_device *steam, bool connected)
-- 
2.54.0

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

[sashiko-bot]

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

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

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

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

HID: steam: Rearrange teardown sequences

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

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

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

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

[ ... ]

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

[PATCH v3 08/10] 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 9dc77cd76edb..298a2c17e112 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);
 }
 
 /*
@@ -1193,6 +1201,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 {
@@ -1839,6 +1848,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 v3 08/10] HID: steam: Improve logging and other cleanup

[sashiko-bot]

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

New issues:
- [High] The patch introduces a kernel stack information leak by logging uninitialized memory in the error path of `steam_get_serial`.

Pre-existing issues:
- [High] `steam_get_serial()` does not verify the length of the received report before reading from the uninitialized `reply` buffer, leaking kernel stack memory into the device serial number.
- [High] Infinite workqueue reschedule loop if the controller disconnects while force feedback is active.
--

--- Patch [8]: [PATCH v3 08/10] HID: steam: Improve logging and other cleanup ---
commit 077388dea9c00bbb5cfcdddffe77daaebbb2a614
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Improve logging and other cleanup

This commit adds more logging as appropriate, reindents an enum to match 
surrounding style, and replaces manual lock/unlock sequences with scope-based 
guard() constructs.

> 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

[ ... ]

> @@ -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) {

[Severity: High]
This is a pre-existing issue, but can this validation block read out-of-bounds
uninitialized memory?

If a device returns a short report (e.g., 1 byte), steam_recv_report()
returns 1, leaving reply[1] and reply[2] uninitialized.

If these uninitialized bytes happen to match the validation criteria, could
strscpy() copy up to 21 bytes of uninitialized stack memory into
steam->serial_no, which is then exposed to userspace via the input subsystem?

> -		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 dmesg?

If steam_recv_report() returns a short report (e.g., fewer bytes than
sizeof(reply)), the reply buffer won't be fully populated.

The newly added hid_err() unconditionally prints the entire sizeof(reply)
array. Does this expose uninitialized kernel stack data to the kernel ring
buffer?

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

[ ... ]

> @@ -1193,6 +1201,7 @@ 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);

[Severity: High]
This is a pre-existing issue, but is there a risk of an infinite loop if the
controller disconnects while force feedback is active?

In steam_coalesce_rumble_cb(), the work continuously reschedules itself based
on the non-zero state of rumble_left or rumble_right:

steam_coalesce_rumble_cb()
    if (steam->rumble_left || steam->rumble_right)
        schedule_delayed_work(&steam->coalesce_rumble_work, HZ / 20);

When steam_unregister() is called on device disconnect, it unregisters the
input devices but doesn't appear to cancel the active force feedback works or
clear their persistent magnitude state.

Could this cause the workqueue to infinitely wake up and send USB reports to
the dongle until it's physically unplugged?

>  	mutex_lock(&steam_devices_lock);

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

[PATCH v3 09/10] 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 298a2c17e112..a80d54ed9784 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

Re: [PATCH v3 09/10] HID: steam: Reject short reads

[sashiko-bot]

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

New issues:
- [Medium] If `hid_hw_raw_request` returns 0, the function prints uninitialized caller stack memory to the kernel debug log.

Pre-existing issues:
- [High] The driver enables hardware events via `hid_hw_start()` before initializing locks and works, leading to TOCTOU and uninitialized lock usage.
- [Medium] The newly added length validation `ret < data[1] + 2` is insufficient for `steam_get_serial`, leaving an uninitialized stack byte read via `strscpy`.
--

--- Patch [9]: [PATCH v3 09/10] HID: steam: Reject short reads ---
commit 9194c57f44b65844dcf5d6df8e1faa2846607590
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: steam: Reject short reads

This commit adds length validation to steam_recv_report() to ensure that the
reported size of a Steam Controller message matches the bytes actually read.
It also addresses stripping the report ID from the buffer more explicitly.

Link: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 298a2c17e1123..a80d54ed9784b 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> @@ -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);

[Severity: Medium]
If hid_hw_raw_request() returns 0, the if (ret > 0) block is bypassed,
leaving size unmodified.  Will this cause hid_dbg() to print uninitialized
stack memory from the data array?

> +	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;
> +	}

[Severity: Medium]
This is a pre-existing issue, but is this length validation sufficient for
steam_get_serial()?

If a device sends a response where the returned length exactly equals
data[1] + 2, steam_recv_report() accepts the packet.

However, steam_get_serial() calls strscpy(..., reply + 3, reply[1]).  This
instructs strscpy() to read up to reply[1] bytes starting from index 3, which
accesses indices 3 through 3 + reply[1] - 1 (which is reply[1] + 2).

Since only indices 0 through data[1] + 1 were validated and initialized, does
this result in an uninitialized stack byte read?

> +	return size;
>  }

[Severity: High]
This is a pre-existing issue, but does steam_probe() enable hardware events
before initializing locks and workqueues?

In drivers/hid/hid-steam.c:steam_probe():

	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
	if (ret)
		return ret;

	steam->hdev = hdev;
	hid_set_drvdata(hdev, steam);
	spin_lock_init(&steam->lock);
	mutex_init(&steam->report_mutex);
	steam->quirks = id->driver_data;
	INIT_WORK(&steam->work_connect, steam_work_connect_cb);

If a connection event arrives immediately from the hardware after
hid_set_drvdata() but before spin_lock_init() or INIT_WORK(), could
steam_raw_event() attempt to acquire an uninitialized spinlock or schedule
an uninitialized work struct?

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

[PATCH v3 10/10] 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 a80d54ed9784..3f5d4a88b864 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];
 
-	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;
@@ -538,14 +570,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

Re: [PATCH v3 10/10] HID: steam: Retry send/recv reports if stale

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized kernel stack memory leaked to dmesg on invalid USB replies or short reads.
- [High] Missing cancellation of workqueues in the `steam_probe` error path leads to a Use-After-Free if probe fails.
--

--- Patch [10]: [PATCH v3 10/10] HID: steam: Retry send/recv reports if stale ---
commit 2a2fc2d99039c81756476abdde17aa7642c354be
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Retry send/recv reports if stale

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.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index a80d54ed9784b..3f5d4a88b8649 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
[ ... ]
> +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);
[ ... ]
> +		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);

[Severity: High]
Does this code leak uninitialized kernel stack memory to dmesg?

If the USB device returns a short reply, steam_recv_report() will only
copy the received bytes into the reply buffer. The remaining bytes in
the buffer remain uninitialized.

Since reply is allocated on the stack in callers like steam_get_serial(),
this hid_err() call will log the full rsize bytes, potentially leaking
sensitive kernel stack memory to dmesg on an invalid or short 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];
>  
> -	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);

[Severity: High]
Similarly, can this leak uninitialized stack memory to dmesg?

If steam_exchange_report() successfully returns but the reply is invalid or
too short, this error path will log the full sizeof(reply), dumping any
uninitialized stack bytes from the reply buffer into dmesg.

>  		return -EIO;
>  	}

[Severity: High]
Does the probe error path cause a Use-After-Free?

Looking at steam_probe() in the same file, if steam_register() fails, the 
probe error path jumps to err_hw_stop:

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

However, this bypasses the cancellation of steam->mode_switch which occurs
under err_steam_unregister:

drivers/hid/hid-steam.c:steam_probe() {
    ...
err_steam_unregister:
	spin_lock_irqsave(&steam->lock, flags);
	steam->client_opened = 0;
	spin_unlock_irqrestore(&steam->lock, flags);
	cancel_work_sync(&steam->work_connect);
	if (steam->connected)
		steam_unregister(steam);
	cancel_delayed_work_sync(&steam->mode_switch);
err_hw_stop:
	cancel_work_sync(&steam->rumble_work);
	cancel_delayed_work_sync(&steam->coalesce_rumble_work);
	hid_hw_stop(hdev);
    ...
}

If a malicious USB device sends an input report that triggers 
steam_do_deck_input_event() before steam_register() fails, the mode_switch 
work is scheduled. When steam_probe() returns the error, the steam struct 
will be freed, but the un-canceled mode_switch delayed work could still 
execute, causing a UAF on the freed struct.

This teardown issue appears to have been introduced by earlier patches in
this series rearranging the teardown paths without properly covering all
workqueues on the error path.

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