Re: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon
From: Guenter Roeck
Date: Tue Aug 04 2026 - 16:14:48 EST
On 8/4/26 11:37, Wilken Gottwalt wrote:
On Tue, 4 Aug 2026 09:34:38 -0700Unless I am missing something there is nothing you can do about this unless there
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote:
Gemini tells me that fixing the raw event problem will require a spinlock to
protect the completion and a separate receive buffer. No idea if it is correct,
but other drivers do the same, so it may have a point. Either case, this is a
bit too much to do without hardware to test, and I'd rather prefer to leave this
up to Wilken.
I was working at that one, too, because I saw Claude Opus hinting on that one.
But it drove me crazy, because every AI is saying something slighty different. I
can not really pin down which one is actually the real solution. I tried to read
through the subsystems code and other drivers, but, argh, I don't know. I was
playing with the idea to (1) remove the raw HID mode completely or (2) make the
driver switchable, raw HID or normal HID, but not both at the same time. On the
other hand, in my Github repo where I develop the driver, I also have a tool
which demonstrates how to access the PSU completely in userspace via libhidpi.
There is actually no need to provide the raw HID access.
Have a look at the patch below. It is part AI (Gemini) generated and part me.
Sashiko is happy with it, but of course that doesn't mean it is perfect or
even correct. It does look good to me, though.
Making Sashiko happy required all core elements of the patch:
- the spinlock
- the separate receive buffer
- the rcv_pending boolean
- the size check in corsairpsu_raw_event()
Sashiko reports race conditions if I drop just one of those elements.
Guenter
---
From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001
From: Guenter Roeck <groeck@xxxxxxxxxx>
Date: Mon, 3 Aug 2026 17:39:21 -0700
Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and
validate reply echo
In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used
both for transmitting command reports via hid_hw_output_report() and for
receiving device responses in corsairpsu_raw_event().
If a command sent via corsairpsu_usb_cmd() times out, the caller stops
waiting, but the hardware may still process the command and send a delayed
response later. If a subsequent command is being prepared or transmitted
when this delayed response arrives, corsairpsu_raw_event() blindly copies
the incoming report into priv->cmd_buffer and completes wait_completion:
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);
This causes a data race where priv->cmd_buffer can be overwritten with
the old delayed response while hid_hw_output_report() is transmitting the
new command, potentially causing the PSU to receive invalid parameters or
shut down. In addition, the caller of the subsequent command will wake up
early and either consume stale data or fail unexpectedly.
Fix the problem by:
- Allocating a separate response buffer (priv->res_buffer) so that
corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing
transfers.
- Validating incoming reports in corsairpsu_raw_event() to ensure that the
echoed length and command opcode match the pending command in
priv->cmd_buffer (or indicate an unsupported command opcode with 0).
- Protecting buffer initialization, reinit_completion(), and report
validation/completion with a spinlock (wait_completion_lock).
- Adding a boolean flag indicating that the code is waiting for a response,
and only copying the reply into the receive buffer if that is the case.
Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxx>
---
drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 5592b927e9d4..3eebf8494ed8 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/types.h>
/*
@@ -122,7 +123,10 @@ struct corsairpsu_data {
struct device *hwmon_dev;
struct dentry *debugfs;
struct completion wait_completion;
+ spinlock_t completion_lock; /* locks wait_completion, cmd_buffer, and res_buffer
*/ u8 *cmd_buffer;
+ u8 *res_buffer;
+ bool rcv_pending;
char vendor[REPLY_SIZE];
char product[REPLY_SIZE];
long temp_crit[TEMP_COUNT];
@@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
{
+ unsigned long flags;
unsigned long time;
int ret;
+ spin_lock_irqsave(&priv->completion_lock, flags);
memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
+ memset(priv->res_buffer, 0, CMD_BUFFER_SIZE);
priv->cmd_buffer[0] = p0;
priv->cmd_buffer[1] = p1;
priv->cmd_buffer[2] = p2;
-
reinit_completion(&priv->wait_completion);
+ priv->rcv_pending = true;
+ spin_unlock_irqrestore(&priv->completion_lock, flags);
ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
if (ret < 0)
@@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1,
u8 p2,
* was send, not every command is supported on every device class, if a command is not
* supported, the length value in the reply is okay, but the command value is set to 0
*/
- if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1])
+ if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1])
return -EOPNOTSUPP;
if (data)
- memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
+ memcpy(data, priv->res_buffer + 2, REPLY_SIZE);
return 0;
}
@@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
hid_device_id if (!priv->cmd_buffer)
return -ENOMEM;
+ priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
+ if (!priv->res_buffer)
+ return -ENOMEM;
+
ret = hid_parse(hdev);
if (ret)
return ret;
@@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
hid_device_id
priv->hdev = hdev;
hid_set_drvdata(hdev, priv);
+ spin_lock_init(&priv->completion_lock);
init_completion(&priv->wait_completion);
hid_device_io_start(hdev);
@@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report
*repo int size)
{
struct corsairpsu_data *priv = hid_get_drvdata(hdev);
+ unsigned long flags;
- if (completion_done(&priv->wait_completion))
+ if (size < 2)
return 0;
- memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
- complete(&priv->wait_completion);
+ spin_lock_irqsave(&priv->completion_lock, flags);
+ if (priv->rcv_pending && !completion_done(&priv->wait_completion) &&
+ data[0] == priv->cmd_buffer[0] &&
+ (data[1] == priv->cmd_buffer[1] || data[1] == 0)) {
+ memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size));
+ complete(&priv->wait_completion);
+ priv->rcv_pending = false;
+ }
+ spin_unlock_irqrestore(&priv->completion_lock, flags);
return 0;
}
I don't know, something is odd about this. I'm not 100% sure, but the lock is
released before hid_hw_output_report(), which is necessary because a spinlock
must not be held by a potentially sleeping call. This leaves a window in which
a delayed reply can satisfy a freshly reinitialized wait_completion() even
before the actual send, if the new command has the same echo (p0/p1) as the old
one. And that is precisely the typical scenario when polling the same sensor
attributes (always 3, 0x8B for rail voltage). Or I just misinterpret this whole
thing entirely, I'm getting tired...
is a sequence number in the message. I think this is why the AI insists that there
is a separate receive buffer (to avoid overwriting the command buffer in this
scenario).
Guenter