[PATCH] hwmon: (corsair-psu) Separate request/response buffers and validate reply echo

[Guenter Roeck]

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;
 }
-- 
2.55.0