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

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

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

--- Patch [2]: [PATCH v5 02/11] HID: steam: Refactor and clean up report parsing ---
commit b341becd37dc80cfee5384e2c463afc0e1624d32
Author: Vicki Pfau <[email protected]>

HID: steam: Refactor and clean up report parsing

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

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

[ ... ]

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

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

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

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

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

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

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

The rumble_work is cancelled before its producer, coalesce_rumble_work:

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2