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

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

On 8/4/26 11:37, Wilken Gottwalt wrote:
> On Tue, 4 Aug 2026 09:34:38 -0700
> Guenter Roeck <linux@roeck-us.net> 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@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;
>>   }
> 
> 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...
> 
Unless I am missing something there is nothing you can do about this unless there
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