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

[sashiko-bot@kernel.org]

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