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

[Guenter Roeck <linux@roeck-us.net>]

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@google.com>
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@kernel.org>
Signed-off-by: Guenter Roeck <groeck@google.com>
---
 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;
 }
-- 
2.55.0