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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared `priv->cmd_buffer` is modified concurrently by process context and interrupt/softirq context without synchronization, leading to data races and hardware state machine corruption.
- [Medium] Missing null termination for USB-provided strings allows an out-of-bounds read in debugfs.
--

--- Patch [1]: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon ---
commit 99da3eb40b38c5c1f4427842c0633cb5ade4ac3c
Author: Ali Ahmet Memis <ali@iusegentoo.com>

hwmon: (corsair-psu) serialize debugfs access against hwmon

This commit adds guard(hwmon_lock)(priv->hwmon_dev) to debugfs read
operations to serialize them against other hwmon subsystem calls. The hwmon
core serializes its own callers, but the debugfs files call
corsairpsu_get_value() directly.

Link: https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
Link: https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/

> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58a..24100519cd834 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this sequence lead to data corruption?

Looking at corsairpsu_usb_cmd(), the completion is reinitialized before
the command is transmitted:

drivers/hwmon/corsair-psu.c:corsairpsu_usb_cmd() {
    ...
	reinit_completion(&priv->wait_completion);

	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
    ...
}

If a delayed or spurious HID report arrives during this window,
corsairpsu_raw_event() will see completion_done() as false:

drivers/hwmon/corsair-psu.c:corsairpsu_raw_event() {
    ...
	if (completion_done(&priv->wait_completion))
		return 0;

	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
	complete(&priv->wait_completion);
    ...
}

Will this asynchronous overwrite corrupt the shared cmd_buffer while it is
being transmitted via hid_hw_output_report(), and cause the driver to process
a delayed reply as the answer to the new command?

[Severity: Medium]
This is a pre-existing issue, but does this copy guarantee null termination
for string fields?

When populating string fields like priv->vendor, 24 bytes (REPLY_SIZE) are
copied directly from the USB response:

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

	return 0;
}

If a non-compliant or malicious USB device provides exactly 24 non-null
characters, there won't be a null terminator.

When the debugfs file is read later, could this result in an out-of-bounds
read that leaks adjacent kernel memory to userspace?

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

	return 0;
}

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