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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added error logging in `steam_get_serial` leaks uninitialized kernel stack memory to the kernel log via `hid_err`.
--

--- Patch [8]: [PATCH v5 08/11] HID: steam: Improve logging and other cleanup ---
commit 15f36444bb8b36e80ca21a45d7b255628f820523
Author: Vicki Pfau <[email protected]>

HID: steam: Improve logging and other cleanup

This patch adds additional logging to the hid-steam driver and aligns
enumeration definitions. It also cleans up several error paths by replacing
manual mutex lock and unlock calls with scope-based guard() protection.

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 663fda8a86fdb..222b5751040a6 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]
Could this leak uninitialized stack memory to the kernel log?

The reply array is allocated on the stack at the start of steam_get_serial()
without being zero-initialized.

If steam_recv_report() reads a short or invalid response, it may return
fewer bytes than the full size of the buffer. By unconditionally printing
sizeof(reply) bytes in this error path, it looks like uninitialized kernel
stack data will be dumped to dmesg.

Would it be safer to print only the number of bytes actually received (ret),
or to zero-initialize the reply array when it is declared?

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

[ ... ]

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