Re: [PATCH] hwmon: (corsair-psu) null terminate the vendor and product strings

[Wilken Gottwalt <wilken.gottwalt@posteo.net>]

On Sun,  2 Aug 2026 12:53:27 +0000
Ali Ahmet Memis <ali@iusegentoo.com> wrote:

> corsairpsu_usb_cmd() copies a fixed REPLY_SIZE bytes out of the reply
> into the caller's buffer:
> 
> 	if (data)
> 		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
> 
> corsairpsu_fwinfo() passes priv->vendor and priv->product, both declared
> as char[REPLY_SIZE]. A device that fills all 24 bytes without a NUL
> leaves them unterminated, and the debugfs files print them with %s:
> 
> 	seq_printf(seqf, "%s\n", priv->vendor);
> 
> The read then runs on into whatever follows in the structure, product
> for vendor and temp_crit[] for product, until it happens to find a zero
> byte. priv comes from devm_kzalloc() so it stays inside the allocation
> and terminates eventually, but the strings are still wrong and the
> contents of neighbouring fields end up in debugfs.
> 
> Give both arrays one more byte. The structure is zero allocated and
> nothing else writes past REPLY_SIZE, so the terminator is always there.
> 
> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> This came up while looking at the driver for the debugfs locking patch
> posted earlier today, and the automated review on that thread flagged it
> too:
> 
>   https://lore.kernel.org/all/20260802123653.19532-1-ali@iusegentoo.com/
> 
> The two are independent; this one applies to master on its own and does
> not depend on the locking change.
> 
> I have no Corsair PSU, so I have not seen a device actually fill all 24
> bytes. The fix is on the grounds that the driver should not depend on
> the device terminating the string.

The device always provides terminated strings. The vendor string ("CORSAIR"
or "Corsair") and the device string (3/4 numbers + 3 letters) are allways
around 10-16 bytes. Actually, it would make more sense to change the memcpy
to "REPLY_SIZE - 1". Just my thought.

greetings Wilken

>  drivers/hwmon/corsair-psu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 24100519cd83..a242373c4656 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -123,8 +123,8 @@ struct corsairpsu_data {
>  	struct dentry *debugfs;
>  	struct completion wait_completion;
>  	u8 *cmd_buffer;
> -	char vendor[REPLY_SIZE];
> -	char product[REPLY_SIZE];
> +	char vendor[REPLY_SIZE + 1];
> +	char product[REPLY_SIZE + 1];
>  	long temp_crit[TEMP_COUNT];
>  	long in_crit[RAIL_COUNT];
>  	long in_lcrit[RAIL_COUNT];
> 
> base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
> prerequisite-patch-id: b3289aa9b605d10f8499c46149f857a9d0a2b2e7