Re: [PATCH 08/10] HID: steam: Improve logging and other cleanup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Self-deadlock in rumble callbacks due to nested acquisition of `steam->report_mutex`.
- [Medium] The newly added error path in `steam_get_serial()` unconditionally prints the uninitialized stack buffer `reply`, leading to a kernel stack information leak.

Pre-existing issues:
- [High] Race condition in `steam_probe` error path with `work_connect` leads to UAF and dangling input devices.
--

commit 8cdb505892ea25a09ef1370038b435985daea2fb
Author: Vicki Pfau <vi@endrift.com>

HID: steam: Improve logging and other cleanup

This commit adds debug logging to report sending and receiving, re-indents
input report identifiers for consistent styling, and converts several mutex
lock/unlock sequences to use the guard(mutex) scoped cleanup pattern.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 9d1fe97921011..593151709cf1d 100644
> --- 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) {
> -		ret = -EIO;
> -		goto out;
> +		hid_err(steam->hdev, "%s: invalid reply (%*ph)\n", __func__,
> +				(int)sizeof(reply), reply);

[Severity: Medium]
Could this unconditionally print the uninitialized reply stack buffer?

steam_get_serial() allocates reply on the stack without initializing it.
steam_recv_report() reads data into reply, but if the device returns a
short response, only part of reply is written.

In this error path, hid_err() uses %*ph to print the entire sizeof(reply)
bytes. Could this expose uninitialized kernel stack memory to dmesg if the
device malfunctions or returns a short response?

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

I also noticed a couple of other issues in this file while reviewing this
patch.

drivers/hid/hid-steam.c:
static void steam_haptic_rumble_cb(struct work_struct *work)
{
	struct steam_device *steam = container_of(work, struct steam_device,
							rumble_work);

	guard(mutex)(&steam->report_mutex);
	steam_haptic_rumble(steam, 0, steam->rumble_left,
		steam->rumble_right, 2, 0);
}

[Severity: High]
I noticed an issue introduced by the "Coalesce rumble packets" patch earlier in
this same series. Does this callback cause a self-deadlock?

Both steam_haptic_rumble_cb() and steam_coalesce_rumble_cb() acquire 
steam->report_mutex via guard(mutex)(&steam->report_mutex) and then immediately
invoke steam_haptic_rumble().

Since steam_haptic_rumble() unconditionally calls 
mutex_lock(&steam->report_mutex) and Linux mutexes are not recursive, won't
this result in a guaranteed self-deadlock of the workqueue thread the first
time a rumble effect is played?


drivers/hid/hid-steam.c:steam_probe() {
    ...
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:
	cancel_work_sync(&steam->work_connect);
    ...
}

[Severity: High]
This is a pre-existing issue, but is there a race condition in this teardown
order?

If steam_probe() fails after requesting wireless connection status, the device
may have already triggered steam_do_connect_event(), which schedules
steam->work_connect.

In the error path above, steam_unregister() is called before the worker is
cancelled with cancel_work_sync(&steam->work_connect).

If the worker is executing concurrently, steam_unregister() may do nothing
because serial_no isn't set yet. The worker would then complete
steam_register() and register the input devices.

After cancel_work_sync() waits for the worker to finish, steam_probe()
returns, freeing the steam struct. Could this leave dangling input devices
that will trigger a use-after-free when accessed?

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