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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Memory leak of devm-allocated battery name if power_supply_register() fails.

Pre-existing issues:
- [High] Race condition in steam_remove leading to Use-After-Free of steam_device.
- [High] List corruption and Memory Leak when steam_register fails during probe.
- [High] Race condition in hidraw open/close callbacks leading to Use-After-Free.
- [High] TOCTOU race in steam_raw_event accessing steam->client_hdev during device removal.
--

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

HID: steam: Fully unregister controller when hidraw is opened

This patch changes the driver to fully unregister the controller internally
when hidraw is opened, rather than just detaching the evdev nodes. This
also unifies the unregister and connect callbacks.

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

[ ... ]

> @@ -1077,6 +1076,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);
>  }

[Severity: Medium]
If power_supply_register() fails in steam_battery_register(), steam->battery
is not set. When this unregister function is called, an early return on
!battery could skip this newly added devm_kfree():

steam_battery_unregister()
{
        ...
        if (!battery)
                return;
        ...
        devm_kfree(&steam->hdev->dev, steam->battery_desc.name);
}

Since this unregister function is now dynamically triggered every time the
hidraw node is opened, could an unprivileged user repeatedly open and close
the hidraw node to continuously leak the devm-allocated string?

[ ... ]

> @@ -1277,7 +1255,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;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but releasing the lock here before
scheduling work appears to open a race window:

steam_client_ll_open()
{
        ...
        spin_unlock_irqrestore(&steam->lock, flags);
        schedule_work(&steam->work_connect);
        ...
}

If the thread is preempted immediately after releasing the lock, and the
physical device is removed concurrently, steam_remove() could execute, free
the steam structure, and leave the resumed schedule_work() call to access
freed memory. Is there a way to safely synchronize this against device
removal?

[ ... ]

> @@ -1389,7 +1367,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);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the error handling
path in steam_probe() since it is closely related to driver registration:

steam_probe()
{
        ...
        ret = steam_register(steam);
        if (ret) {
                hid_err(hdev, ...);
                goto err_hw_close;
        }
        ...
}

If steam_register() adds the device to the steam_devices list, but a
subsequent step like steam_input_register() fails, steam_register() returns
an error but leaves the device on the global list.

Since the err_hw_close path bypasses unregistering, could this lead to a
use-after-free when the lizard_mode module parameter later traverses the
steam_devices list?

[ ... ]

> @@ -1470,7 +1446,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 calling cancel_work_sync() here before
stopping the hardware leave a race window?

steam_remove()
{
        ...
        cancel_work_sync(&steam->work_connect);
        ...
} // hid_hw_stop() is called later

Because the HID hardware is still active, could an incoming connection event
interrupt trigger steam_raw_event() and re-schedule work_connect after it was
canceled?

Once steam_remove() completes and frees the steam object, this might cause the
workqueue to execute the asynchronous callback using the freed steam structure.

[Severity: High]
This is also a pre-existing issue in the raw event handler, but could this
unlocked check lead to a time-of-check to time-of-use race?

steam_raw_event()
{
        ...
        if (steam->client_opened)
                hid_input_report(steam->client_hdev, ...);
        ...
}

If the thread is preempted after reading a non-zero client_opened value, and
the physical device is concurrently unplugged, steam_remove() could run and
destroy client_hdev.

When steam_raw_event() resumes, will it attempt to call hid_input_report()
with the freed steam->client_hdev pointer?

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