Re: [PATCH v2 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] Uninitialized kernel stack memory leaked to dmesg via short reports in steam_get_serial()
- [High] Use-After-Free due to bypassed workqueue cancellation in steam_probe() error paths

Pre-existing issues:
- [High] Use-After-Free via global steam_devices list due to bypassed unregister in probe error paths
--

--- Patch [8]: [PATCH v2 08/10] HID: steam: Improve logging and other cleanup ---
commit 5aa379e88890315b198ed7d8da2c352e3a8ce6a8
Author: Vicki Pfau <vi@endrift.com>

    HID: steam: Improve logging and other cleanup
    
    This commit adds more logging where appropriate and reindents an enum to
    match the surrounding coding style. It also cleans up some places where
    guard() can be used for lock management instead of manually calling locking
    and unlocking functions.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 296ced2989a42..1f2cf15d40831 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: High]
Does this code risk exposing uninitialized kernel stack memory to the kernel
log?

When steam_recv_report() reads a short report, it might copy fewer bytes than
sizeof(reply) into the uninitialized reply buffer. The new hid_err() call
unconditionally dumps the entire buffer capacity, which could leak stack
memory if the device responds with an invalid or short report.

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

[ ... ]

While reviewing this series, I also noticed a couple of issues in the
error handling paths of steam_probe() from earlier patches or existing code:

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer in
the global steam_devices list on registration failure?

In steam_probe():

	} else {
		/* A wired connection is always present */
		steam->connected = true;
		ret = steam_register(steam);
		if (ret) {
			hid_err(hdev,
				"%s:steam_register failed with error %d\n",
				__func__, ret);
			goto err_hw_close;
		}
	}

If steam_register() fails, the device is already added to the global
steam_devices list, but the error path jumps to err_hw_close. This bypasses
err_steam_unregister where it would normally be removed.

Once probe fails, devm frees the steam structure, which can lead to a
use-after-free if the lizard_mode sysfs parameter is subsequently accessed.


[Severity: High]
Does the following sequence result in a use-after-free if probe fails late?

In a previous patch in this series, the cancellation of work_connect was
moved from err_cancel_work to err_steam_unregister:

err_steam_unregister:
	spin_lock_irqsave(&steam->lock, flags);
	steam->client_opened = 0;
	spin_unlock_irqrestore(&steam->lock, flags);
	cancel_work_sync(&steam->work_connect);
	if (steam->connected)
		steam_unregister(steam);
err_hw_close:
	hid_hw_close(hdev);
err_hw_stop:
	hid_hw_stop(hdev);
err_cancel_work:
	cancel_delayed_work_sync(&steam->mode_switch);
	cancel_work_sync(&steam->rumble_work);
	cancel_delayed_work_sync(&steam->coalesce_rumble_work);

If an ID_CONTROLLER_WIRELESS event is received early asynchronously,
work_connect could be queued. If hid_hw_open() then fails and jumps to
err_hw_stop, err_steam_unregister is bypassed.

The devm subsystem will free the steam structure, and the still-queued work
item could crash when executed.

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