Re: [PATCH v2] hwmon: (corsair-psu) serialize debugfs access against hwmon

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

Pre-existing issues:
- [High] Potential data race and buffer corruption on the shared `cmd_buffer` following a command timeout.
- [High] Out-of-bounds read and potential information leak in debugfs handlers for vendor and product strings.
--

--- Patch [1]: [PATCH v2] hwmon: (corsair-psu) serialize debugfs access against hwmon ---
commit 17027f95f57d41549658af1b49c4295c03ed4860
Author: Ali Ahmet Memis <[email protected]>

hwmon: (corsair-psu) serialize debugfs access against hwmon
    
This commit adds guard(hwmon_lock) to the debugfs read paths in 
print_uptime() and ocpmode_show() to serialize them against the hwmon core.
This prevents data races and incorrect sensor readings resulting from
concurrent USB commands interleaving.

Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58a..033166db6bc42 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
>  	long val;
>  	int ret;
>  
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, cmd, 0, &val);

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a data race or
buffer corruption on the shared cmd_buffer if a USB command times out?

If wait_for_completion_timeout() times out in corsairpsu_usb_cmd(), the
completion is left uncompleted. If the hardware later sends a delayed
response, the asynchronous HID interrupt in corsairpsu_raw_event() will
blindly copy the data:

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

If a new request is concurrently executing in corsairpsu_usb_cmd(), this
delayed copy will race with its setup:

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


[Severity: High]
This is a pre-existing issue, but does this code potentially cause an
out-of-bounds read and information leak in the debugfs handlers?

When corsairpsu_fwinfo() fetches the vendor and product information, it
copies exactly REPLY_SIZE (24) bytes from the untrusted USB device into
priv->vendor and priv->product:

drivers/hwmon/corsair-psu.c:corsairpsu_usb_cmd() {
    ...
    memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
    ...
}

If a malicious device provides exactly 24 non-null characters, these strings
will not be null-terminated. When seq_printf() prints them in vendor_show()
and product_show(), it could read past the array bounds and leak adjacent
kernel heap memory:

drivers/hwmon/corsair-psu.c:vendor_show() {
    ...
    seq_printf(seqf, "%s\n", priv->vendor);
    ...
}

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