Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Battery registration fails due to thermal zone name length limits if the device serial number is long.
- [Medium] Incorrect initialization of battery temperature results in reporting 0.2 °C.
- [Low] Duplicate error logging block introduced in steam_recv_report_id.
Pre-existing issues:
- [High] Probe failure leaves a registered power supply with dangling devm pointers.
- [High] Unbinding the driver while the device is active can lead to a Use-After-Free in steam_work_connect_cb.
--
--- Patch [1]: [PATCH 1/5] HID: steam: Initial 2026 Steam Controller support ---
commit 10f5a4093529f8973ab3200723fc61d0c419c6bb
Author: Vicki Pfau [off-list ref]
HID: steam: Initial 2026 Steam Controller support
This brings support for the 2026 Steam Controller, comparably featureful to
the existing support for the Steam Deck.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 6199f67f3c4cd..111e57926d049 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -378,21 +459,38 @@ static int steam_recv_report(struct steam_device *steam,
return -ENOMEM;
/*
- * The report ID is always 0, so strip the first byte from the output.
+ * The report ID is consistent, so strip the first byte from the output.
* hid_report_len() is not counting the report ID, so +1 to the length
* or else we get a EOVERFLOW. We are safe from a buffer overflow
* because hid_alloc_report_buf() allocates +7 bytes.
*/
- ret = hid_hw_raw_request(steam->hdev, 0x00,
- buf, hid_report_len(r) + 1,
- HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ if (!(steam->quirks & STEAM_QUIRK_IBEX))
+ len += 1;
+
+ /*
+ * Sometimes the wireless controller fails with EPIPE
+ * when sending a feature report.
+ * Doing a HID_REQ_GET_REPORT and waiting for a while
+ * seems to fix that.
+ */
+ do {
+ ret = hid_hw_raw_request(steam->hdev, report_id,
+ buf, len,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ if (ret != -EPIPE)
+ break;
+ msleep(20);
+ } while (--retries);
if (ret > 0) {
/* 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);
if (ret < 0)
hid_err(steam->hdev, "%s: error %d\n", __func__, ret);
[Severity: Low]
Is this error logging block accidentally duplicated? There are two identical
checks and hid_err() calls sequentially in steam_recv_report_id(), which will
cause the exact same error message to be printed twice upon a single
hid_hw_raw_request() failure.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -854,19 +1051,35 @@ static int steam_battery_register(struct steam_device *steam)
int ret;
steam->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
- steam->battery_desc.properties = steam_battery_props;
- steam->battery_desc.num_properties = ARRAY_SIZE(steam_battery_props);
+ if (steam->quirks & STEAM_QUIRK_IBEX) {
+ steam->battery_desc.properties = steam_ibex_battery_props;
+ steam->battery_desc.num_properties = ARRAY_SIZE(steam_ibex_battery_props);
+ /*
+ * Ibex needs a shorter name as it has a temperature and the
+ * thermal zone name length limit is 20 characters. It's more
+ * ambiguous sounding, so let's only use it when needed.
+ */
+ steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
+ GFP_KERNEL, "steam-%s",
+ steam->serial_no);
[Severity: High]
Will this name exceed the 20-character maximum permitted for a thermal zone
device type if the serial number is long?
Because STEAM_SERIAL_LEN is 21, a serial number of 14 characters or
longer combined with the "steam-" prefix will exceed 20 characters. This
causes thermal_zone_device_register_with_trips() to return -EINVAL,
resulting in power_supply_register() and steam_battery_register() failing
entirely.
quoted hunk ↗ jump to hunk
+ } else {
+ steam->battery_desc.properties = steam_battery_props;
+ steam->battery_desc.num_properties = ARRAY_SIZE(steam_battery_props);
+ steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
+ GFP_KERNEL, "steam-controller-%s-battery",
+ steam->serial_no);
+ }
steam->battery_desc.get_property = steam_battery_get_property;
- steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
- GFP_KERNEL, "steam-controller-%s-battery",
- steam->serial_no);
if (!steam->battery_desc.name)
return -ENOMEM;
/* avoid the warning of 0% battery while waiting for the first info */
spin_lock_irqsave(&steam->lock, flags);
- steam->voltage = 3000;
+ steam->battery_voltage = 3000;
steam->battery_charge = 100;
+ steam->battery_current = 0;
+ steam->battery_temp = 200;
[Severity: Medium]
Does this initialize the default temperature to 0.2 degrees Celsius?
In steam_battery_get_property(), this value is divided by 100:
val->intval = temp / 100;
Since the power supply subsystem expects POWER_SUPPLY_PROP_TEMP in tenths
of a degree Celsius, returning 2 represents 0.2 C. If the intended default
temperature was 20.0 C, should this be initialized to 20000?
quoted hunk ↗ jump to hunk
+ steam->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
spin_unlock_irqrestore(&steam->lock, flags);
battery = power_supply_register(&steam->hdev->dev,
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1149,51 +1380,42 @@ static int steam_register(struct steam_device *steam)
- spin_lock_irqsave(&steam->lock, flags);
- client_opened = steam->client_opened;
- spin_unlock_irqrestore(&steam->lock, flags);
+ /* ignore battery errors, we can live without it */
+ if (steam->quirks & (STEAM_QUIRK_WIRELESS | STEAM_QUIRK_IBEX))
+ steam_battery_register(steam);
- if (!client_opened) {
- steam_set_lizard_mode(steam, lizard_mode);
- ret = steam_input_register(steam);
- if (ret != 0)
- goto steam_register_input_fail;
- ret = steam_sensors_register(steam);
- if (ret != 0)
- goto steam_register_sensors_fail;
- }
+ steam_set_lizard_mode(steam, lizard_mode);
+ ret = steam_input_register(steam);
+ if (ret != 0)
+ goto steam_register_input_fail;
+ ret = steam_sensors_register(steam);
+ if (ret != 0)
+ goto steam_register_sensors_fail;
[Severity: High]
This is a pre-existing issue, but does this error path leave the battery
registered if steam_input_register() or steam_sensors_register() fails?
If steam_register() fails, steam_probe() jumps to the err_hw_close label,
which bypasses steam_unregister(). Because steam_probe() returns an error,
devres will free the steam_device structure.
Since the battery was not unregistered, it remains active in sysfs, and any
subsequent userspace read of the battery's properties will dereference the
freed steam_device pointer, causing a kernel use-after-free.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1989,8 +2462,84 @@ static int steam_raw_event(struct hid_device *hdev,
return 0;
if (steam->client_opened)
- hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
- data, size, 0);
+ hid_input_report(steam->client_hdev, report->type, data, size, 0);
+
+ /* Ibex uses a different report format */
+ if (steam->quirks & STEAM_QUIRK_IBEX) {
+ if (report->type != HID_INPUT_REPORT)
+ return 0;
+
+ switch (report->id) {
+ case REPORT_ID_INPUT:
+ if (size != 54)
[ ... ]
quoted hunk ↗ jump to hunk
+ case REPORT_ID_WIRELESS_EVENT:
+ if (size != 2)
+ return 0;
+ switch (data[1]) {
+ case WIRELESS_EVENT_DISCONNECT:
+ steam_do_connect_event(steam, false);
+ break;
+ case WIRELESS_EVENT_CONNECT:
+ steam_do_connect_event(steam, true);
+ break;
+ }
+ break;
[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free during
driver unbind?
When the driver is unbound, steam_remove() cancels the steam->work_connect
work item and calls steam_unregister(), but hid_hw_stop() is not called
until the very end.
In this window, the device can still stream data and trigger
steam_raw_event(), which can call steam_do_connect_event(). This
reschedules steam->work_connect back onto the workqueue. Once
steam_remove() returns, devres frees the steam_device structure, causing
the scheduled work item to execute steam_work_connect_cb() on freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805015116.3884465-1-vi@endrift.com?part=1