Re: [PATCH v3 02/10] HID: steam: Refactor and clean up report parsing

[sashiko-bot@kernel.org]

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