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

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

[Ali Ahmet Memis]

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.

 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
-- 
2.55.0

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

[Wilken Gottwalt]

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

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

[Ali Ahmet Memis]

On Sun, Aug 02 2026, Wilken Gottwalt wrote:

> 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.

Thanks, that settles it. So there is no reachable bug here, only a driver
that relies on the device terminating the string. That changes what the
patch should look like: v2 will say that outright, and I will drop the
Fixes: tag, since this is hardening rather than a fix and it has no
business going to stable.

> Actually, it would make more sense to change the memcpy to
> "REPLY_SIZE - 1". Just my thought.

That works. One detail worth weighing before you pick: corsairpsu_usb_cmd()
is also reached with a non NULL data from corsairpsu_request(), and
corsairpsu_get_value() passes an uninitialized u8 data[REPLY_SIZE] on the
stack. With REPLY_SIZE - 1 the last byte of that buffer is never written.
Only data[0..3] are read, so nothing breaks, but the shortening is not
confined to the two string callers.

Either is fine by me and it is your driver, so say which you prefer and I
will send v2 that way:

  a) memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE - 1), arrays unchanged
  b) vendor and product as char[REPLY_SIZE + 1], memcpy unchanged

No rush on the locking patch either, whenever you get to testing it.

--
Ali

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

[Guenter Roeck]

On 8/2/26 06:42, Ali Ahmet Memis wrote:
> On Sun, Aug 02 2026, Wilken Gottwalt wrote:
> 
>> 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.
> 
> Thanks, that settles it. So there is no reachable bug here, only a driver
> that relies on the device terminating the string. That changes what the
> patch should look like: v2 will say that outright, and I will drop the
> Fixes: tag, since this is hardening rather than a fix and it has no
> business going to stable.
> 
>> Actually, it would make more sense to change the memcpy to
>> "REPLY_SIZE - 1". Just my thought.
> 
> That works. One detail worth weighing before you pick: corsairpsu_usb_cmd()
> is also reached with a non NULL data from corsairpsu_request(), and
> corsairpsu_get_value() passes an uninitialized u8 data[REPLY_SIZE] on the
> stack. With REPLY_SIZE - 1 the last byte of that buffer is never written.
> Only data[0..3] are read, so nothing breaks, but the shortening is not
> confined to the two string callers.
> 
> Either is fine by me and it is your driver, so say which you prefer and I
> will send v2 that way:
> 
>    a) memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE - 1), arrays unchanged
>    b) vendor and product as char[REPLY_SIZE + 1], memcpy unchanged
> 
> No rush on the locking patch either, whenever you get to testing it.
> 
Again, please stop fixing non-issues. We have enough real issues to deal with.

Thanks,
Guenter

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

[Ali Ahmet Memis]

Understood. Dropping it, there will be no v2.

I should have said that in my previous mail instead of offering options
after agreeing there was no bug to fix.

--
Ali

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Debugfs attributes lack hwmon_lock() serialization, causing race conditions with sysfs operations on shared command buffers.
- [High] The mantissa sign-extension in `corsairpsu_linear11_to_int` fails due to C integer promotion rules, causing negative mantissa values to be parsed incorrectly as massive positive values.
- [High] The driver shares a single buffer (`priv->cmd_buffer`) between the TX path and asynchronous RX path without proper separation, leading to data races and DMA corruption.
--

--- Patch [1]: [PATCH] hwmon: (corsair-psu) null terminate the vendor and product strings ---
commit b38110dc992661872091c42f387566734213a5a4
Author: Ali Ahmet Memis <ali@iusegentoo.com>

hwmon: (corsair-psu) null terminate the vendor and product strings

This commit increases the vendor and product arrays by one byte. Because
the structure is zero-allocated, this ensures the strings are always
null-terminated even if the device fills the entire buffer.

> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58a..5852f3e4474c8 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;

[Severity: High]
This is a pre-existing issue, but does the lack of hwmon_lock()
serialization in debugfs functions cause race conditions?

Concurrent reads of debugfs attributes and sysfs attributes bypass the hwmon
core's serialization. For example:

drivers/hwmon/corsair-psu.c:uptime_show() {
    print_uptime(seqf, PSU_CMD_UPTIME);
    ...
}

This invokes USB commands without acquiring hwmon_lock(priv->hwmon_dev) or
an internal driver lock, which can race with sysfs operations:

drivers/hwmon/corsair-psu.c:corsairpsu_usb_cmd() {
    memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
    ...
}

Could this data race on priv->cmd_buffer and wait_completion cause malformed
commands to be sent to the PSU hardware, potentially leading to undefined
device behavior or unexpected power down?

[Severity: High]
This is another pre-existing issue, but is it possible for the shared
priv->cmd_buffer to lead to data races and DMA corruption between the TX and
RX paths?

The HID event handler runs asynchronously in softirq context and overwrites
the buffer locklessly:

drivers/hwmon/corsair-psu.c:corsairpsu_raw_event() {
    memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
    ...
}

This can interrupt and corrupt the transmission of the shared command buffer:

drivers/hwmon/corsair-psu.c:corsairpsu_usb_cmd() {
    ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
    ...
}

If a delayed response arrives precisely when a new command is being
transmitted, the outgoing buffer could be corrupted mid-flight. Would this
send a garbled command to the PSU hardware and violate DMA mapping rules if
the USB host controller is actively reading the buffer?

> -	char vendor[REPLY_SIZE];
> -	char product[REPLY_SIZE];
> +	char vendor[REPLY_SIZE + 1];
> +	char product[REPLY_SIZE + 1];
>  	long temp_crit[TEMP_COUNT];

[Severity: High]
This is a pre-existing issue, but does the mantissa sign-extension fail when
parsing temperatures?

Looking at corsairpsu_linear11_to_int():

drivers/hwmon/corsair-psu.c:corsairpsu_linear11_to_int() {
    const int mant = (((s16)(val & 0x7ff)) << 5) >> 5;
    ...
}

This operation fails to sign-extend the 11-bit two's complement mantissa
because the value cast to s16 is bounded by 0x7ff, resulting in a strictly
positive integer after promotion and left shift.

A negative sensor reading would be parsed as a massive positive value.
Because the driver registers with the thermal subsystem, could this massive
temperature reading immediately trigger a thermal trip and cause an
unexpected emergency system shutdown?

>  	long in_crit[RAIL_COUNT];
>  	long in_lcrit[RAIL_COUNT];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802125327.21469-1-ali@iusegentoo.com?part=1