[PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe

[PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe

[Guenter Roeck]

waterforce_raw_event() is the HID ->raw_event() callback and acquires
priv->status_report_request_lock with a plain spin_lock(), while
waterforce_get_status() acquires the same lock with spin_lock_bh().

Depending on the HID transport and USB host controller driver,
waterforce_raw_event() can run in process context (e.g. via uhid with
softirqs and hardirqs enabled), softirq context (USB HCDs using HCD_BH),
or hard interrupt context (USB HCDs without HCD_BH, such as OHCI/UHCI).

Because spin_lock_bh() in waterforce_get_status() leaves hard interrupts
enabled, a hard-IRQ URB completion invoking waterforce_raw_event() on the
same CPU will self-deadlock on status_report_request_lock. Likewise, a
plain spin_lock() in waterforce_raw_event() when called from process
context via uhid leaves both softirqs and hardirqs enabled, risking a
self-deadlock if interrupted by softirq or hardirq raw-event delivery on
the same CPU.

Use spin_lock_irq() in waterforce_get_status(), which always runs in
process context, and spin_lock_irqsave() in waterforce_raw_event(), which
may run with interrupts either enabled or disabled.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 41c71105a845 ("hwmon: (gigabyte_waterforce) Mark status report as received under a spinlock")
Cc: Aleksa Savic <savicaleksa83@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gigabyte_waterforce.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index f4d0376dd68e..65169dbb91d8 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -139,9 +139,9 @@ static int waterforce_get_status(struct waterforce_data *priv)
 	 * the raw event parsing and marked the priv->status_report_received
 	 * completion as done.
 	 */
-	spin_lock_bh(&priv->status_report_request_lock);
+	spin_lock_irq(&priv->status_report_request_lock);
 	reinit_completion(&priv->status_report_received);
-	spin_unlock_bh(&priv->status_report_request_lock);
+	spin_unlock_irq(&priv->status_report_request_lock);
 
 	/* Send command for getting status */
 	ret = waterforce_write_expanded(priv, get_status_cmd, GET_STATUS_CMD_LENGTH);
@@ -256,6 +256,7 @@ static int waterforce_raw_event(struct hid_device *hdev, struct hid_report *repo
 				int size)
 {
 	struct waterforce_data *priv = hid_get_drvdata(hdev);
+	unsigned long flags;
 
 	if (size <= FIRMWARE_VER_START_OFFSET_2)
 		return 0;
@@ -281,10 +282,10 @@ static int waterforce_raw_event(struct hid_device *hdev, struct hid_report *repo
 	priv->duty_input[0] = data[WATERFORCE_FAN_DUTY];
 	priv->duty_input[1] = data[WATERFORCE_PUMP_DUTY];
 
-	spin_lock(&priv->status_report_request_lock);
+	spin_lock_irqsave(&priv->status_report_request_lock, flags);
 	if (!completion_done(&priv->status_report_received))
 		complete_all(&priv->status_report_received);
-	spin_unlock(&priv->status_report_request_lock);
+	spin_unlock_irqrestore(&priv->status_report_request_lock, flags);
 
 	priv->updated = jiffies;
 
-- 
2.45.2

[PATCH 2/2] hwmon: (gigabyte_waterforce) Protect status report data with spinlock

[Guenter Roeck]

waterforce_raw_event() updates temp_input, speed_input, duty_input, and
the updated timestamp without holding priv->status_report_request_lock
(and updates priv->updated after complete_all()), while
waterforce_get_status() and waterforce_read() read those fields without
synchronization.

Because HID_CONNECT_HIDRAW is enabled, or when an interrupted wait leaves
a requested report in flight, waterforce_raw_event() can execute
concurrently with sysfs reads. Under the Linux kernel memory model this
constitutes a data race, lacks memory ordering between priv->updated and
the cached sensor fields on weakly ordered architectures, and allows a
reader woken by complete_all() to re-enter waterforce_get_status() before
priv->updated is refreshed.

Extend priv->status_report_request_lock to cover updating the sensor
values and priv->updated in waterforce_raw_event(), checking priv->updated
in waterforce_get_status(), and reading the cached sensor values in
waterforce_read().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 42ac68e3d4ba ("hwmon: Add driver for Gigabyte AORUS Waterforce AIO coolers")
Cc: Aleksa Savic <savicaleksa83@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gigabyte_waterforce.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 65169dbb91d8..34471852fe21 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -55,7 +55,7 @@ struct waterforce_data {
 	struct mutex buffer_lock;
 	/* For queueing multiple readers */
 	struct mutex status_report_request_mutex;
-	/* For reinitializing the completion below */
+	/* Protects status report data, timestamp, and completion below */
 	spinlock_t status_report_request_lock;
 	struct completion status_report_received;
 	struct completion fw_version_processed;
@@ -128,8 +128,10 @@ static int waterforce_get_status(struct waterforce_data *priv)
 	if (ret < 0)
 		return ret;
 
+	spin_lock_irq(&priv->status_report_request_lock);
 	if (!time_after(jiffies, priv->updated + msecs_to_jiffies(STATUS_VALIDITY))) {
 		/* Data is up to date */
+		spin_unlock_irq(&priv->status_report_request_lock);
 		goto unlock_and_return;
 	}
 
@@ -139,7 +141,6 @@ static int waterforce_get_status(struct waterforce_data *priv)
 	 * the raw event parsing and marked the priv->status_report_received
 	 * completion as done.
 	 */
-	spin_lock_irq(&priv->status_report_request_lock);
 	reinit_completion(&priv->status_report_received);
 	spin_unlock_irq(&priv->status_report_request_lock);
 
@@ -170,6 +171,8 @@ static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
 	if (ret < 0)
 		return ret;
 
+	ret = 0;
+	spin_lock_irq(&priv->status_report_request_lock);
 	switch (type) {
 	case hwmon_temp:
 		*val = priv->temp_input[channel];
@@ -183,14 +186,17 @@ static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
 			*val = DIV_ROUND_CLOSEST(priv->duty_input[channel] * 255, 100);
 			break;
 		default:
-			return -EOPNOTSUPP;
+			ret = -EOPNOTSUPP;
+			break;
 		}
 		break;
 	default:
-		return -EOPNOTSUPP;	/* unreachable */
+		ret = -EOPNOTSUPP;	/* unreachable */
+		break;
 	}
+	spin_unlock_irq(&priv->status_report_request_lock);
 
-	return 0;
+	return ret;
 }
 
 static int waterforce_read_string(struct device *dev, enum hwmon_sensor_types type,
@@ -276,19 +282,17 @@ static int waterforce_raw_event(struct hid_device *hdev, struct hid_report *repo
 	if (size <= WATERFORCE_TEMP_SENSOR)
 		return 0;
 
+	spin_lock_irqsave(&priv->status_report_request_lock, flags);
 	priv->temp_input[0] = data[WATERFORCE_TEMP_SENSOR] * 1000;
 	priv->speed_input[0] = get_unaligned_le16(data + WATERFORCE_FAN_SPEED);
 	priv->speed_input[1] = get_unaligned_le16(data + WATERFORCE_PUMP_SPEED);
 	priv->duty_input[0] = data[WATERFORCE_FAN_DUTY];
 	priv->duty_input[1] = data[WATERFORCE_PUMP_DUTY];
-
-	spin_lock_irqsave(&priv->status_report_request_lock, flags);
+	priv->updated = jiffies;
 	if (!completion_done(&priv->status_report_received))
 		complete_all(&priv->status_report_received);
 	spin_unlock_irqrestore(&priv->status_report_request_lock, flags);
 
-	priv->updated = jiffies;
-
 	return 0;
 }
 
-- 
2.45.2

Re: [PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe

[sashiko-bot]

> waterforce_raw_event() is the HID ->raw_event() callback and acquires
> priv->status_report_request_lock with a plain spin_lock(), while
> waterforce_get_status() acquires the same lock with spin_lock_bh().
> 
> Depending on the HID transport and USB host controller driver,
> waterforce_raw_event() can run in process context (e.g. via uhid with
> softirqs and hardirqs enabled), softirq context (USB HCDs using HCD_BH),
> or hard interrupt context (USB HCDs without HCD_BH, such as OHCI/UHCI).
> 
> Because spin_lock_bh() in waterforce_get_status() leaves hard interrupts
> enabled, a hard-IRQ URB completion invoking waterforce_raw_event() on the
> same CPU will self-deadlock on status_report_request_lock. Likewise, a
> plain spin_lock() in waterforce_raw_event() when called from process
> context via uhid leaves both softirqs and hardirqs enabled, risking a
> self-deadlock if interrupted by softirq or hardirq raw-event delivery on
> the same CPU.
> 
> Use spin_lock_irq() in waterforce_get_status(), which always runs in
> process context, and spin_lock_irqsave() in waterforce_raw_event(), which
> may run with interrupts either enabled or disabled.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: 41c71105a845 ("hwmon: (gigabyte_waterforce) Mark status report as received under a spinlock")
> Cc: Aleksa Savic <savicaleksa83@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031223131.1855967-1-linux@roeck-us.net?part=1

Re: [PATCH 2/2] hwmon: (gigabyte_waterforce) Protect status report data with spinlock

[sashiko-bot]

> waterforce_raw_event() updates temp_input, speed_input, duty_input, and
> the updated timestamp without holding priv->status_report_request_lock
> (and updates priv->updated after complete_all()), while
> waterforce_get_status() and waterforce_read() read those fields without
> synchronization.
> 
> Because HID_CONNECT_HIDRAW is enabled, or when an interrupted wait leaves
> a requested report in flight, waterforce_raw_event() can execute
> concurrently with sysfs reads. Under the Linux kernel memory model this
> constitutes a data race, lacks memory ordering between priv->updated and
> the cached sensor fields on weakly ordered architectures, and allows a
> reader woken by complete_all() to re-enter waterforce_get_status() before
> priv->updated is refreshed.
> 
> Extend priv->status_report_request_lock to cover updating the sensor
> values and priv->updated in waterforce_raw_event(), checking priv->updated
> in waterforce_get_status(), and reading the cached sensor values in
> waterforce_read().
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: 42ac68e3d4ba ("hwmon: Add driver for Gigabyte AORUS Waterforce AIO coolers")
> Cc: Aleksa Savic <savicaleksa83@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031223131.1855967-1-linux@roeck-us.net?part=2