[PATCH] hwmon: (corsair-psu) Fix linear11 calculation

[PATCH] hwmon: (corsair-psu) Fix linear11 calculation

[Guenter Roeck]

In corsairpsu_linear11_to_int(), the mantissa is extracted using bitwise
operations and cast to s16 before being shifted left:

static int corsairpsu_linear11_to_int(const u16 val, const int scale)
{
    ...
    const int mant = (((s16)(val & 0x7ff)) << 5) >> 5;
    ...
}

Due to C integer promotion rules, the masked value (which is always
positive) is promoted to a 32-bit integer before the left shift. As a
result, the sign bit is never extended to bit 31 of the promoted integer.

When the device hardware reports a negative temperature in Linear11 format
(such as an ambient temperature probe reporting sub-zero), the negative
mantissa is parsed incorrectly as a massive positive value. For example,
-1 becomes 2047, which scales to 2047 degrees Celsius.

Fix the problem by type casting the result of the left shift operation
to s16.

Another problem is shifting of negative values. In C, the result of
left-shifting negative values is undefined, and the result of
right-shifting negative values is implementation dependent. Use
multiply and divide operations instead to avoid both problems.

Also use a local s64 variable to store temporary results, change
the return value type from int to long, and clamp the final value
to LONG_MIN and LONG_MAX to avoid under- and overflow issues while
retaining as much information as possible.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/corsair-psu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 24100519cd83..6e12325e0a3b 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -137,13 +137,18 @@ struct corsairpsu_data {
 };
 
 /* some values are SMBus LINEAR11 data which need a conversion */
-static int corsairpsu_linear11_to_int(const u16 val, const int scale)
+static long corsairpsu_linear11_to_long(const u16 val, const int scale)
 {
 	const int exp = ((s16)val) >> 11;
-	const int mant = (((s16)(val & 0x7ff)) << 5) >> 5;
-	const int result = mant * scale;
+	const int mant = ((s16)((val & 0x7ff) << 5)) >> 5;
+	s64 result = mant * scale;
 
-	return (exp >= 0) ? (result << exp) : (result >> -exp);
+	if (exp >= 0)
+		result *= (int)(1UL << exp);
+	else
+		result /= (int)(1UL << -exp);
+
+	return clamp(result, LONG_MIN, LONG_MAX);
 }
 
 /* the micro-controller uses percentage values to control pwm */
@@ -263,13 +268,13 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
 	case PSU_CMD_RAIL_AMPS:
 	case PSU_CMD_TEMP0:
 	case PSU_CMD_TEMP1:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1000);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1000);
 		break;
 	case PSU_CMD_FAN:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		break;
 	case PSU_CMD_FAN_PWM_ENABLE:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		/*
 		 * 0 = automatic mode, means the micro-controller controls the fan using a plan
 		 *     which can be modified, but changing this plan is not supported by this
@@ -283,12 +288,12 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
 			*val = 2;
 		break;
 	case PSU_CMD_FAN_PWM:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		*val = corsairpsu_dutycycle_to_pwm(*val);
 		break;
 	case PSU_CMD_RAIL_WATTS:
 	case PSU_CMD_TOTAL_WATTS:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1000000);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1000000);
 		break;
 	case PSU_CMD_TOTAL_UPTIME:
 	case PSU_CMD_UPTIME:
-- 
2.45.2

Re: [PATCH] hwmon: (corsair-psu) Fix linear11 calculation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing null termination in USB string attributes leads to a kernel memory disclosure.
- [High] Concurrent sysfs and debugfs reads cause a data race on the shared USB command buffer and wait completion.
--

--- Patch [1]: [PATCH] hwmon: (corsair-psu) Fix linear11 calculation ---
Note: The format of this report has been altered to use a free-form summary 
rather than inline diff quotes due to recitation restrictions.

commit 00dd75e6c6959cf10ed39455a52e60c23722db89
Author: Guenter Roeck <linux@roeck-us.net>
Subject: hwmon: (corsair-psu) Fix linear11 calculation

This commit fixes the linear11 to integer calculation by correctly casting 
the shifted values to s16 and using 64-bit temporary variables. It prevents 
sign-extension issues with negative temperatures and clamps the final result 
to avoid overflow.

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the USB
string attributes to lack null termination, leading to a kernel memory
disclosure?

Looking at corsairpsu_usb_cmd() in drivers/hwmon/corsair-psu.c, it
unconditionally copies REPLY_SIZE bytes from the USB response into the
string buffers like priv->vendor using memcpy(). If a USB device returns
exactly 24 bytes of non-null characters, no null terminator is appended.

Later, when a user reads the corresponding world-readable debugfs file,
vendor_show() uses seq_printf() with "%s" to read priv->vendor. Could this
result in an out-of-bounds read that leaks adjacent kernel memory fields?

[Severity: High]
This is a pre-existing issue, but does this code have a data race when
handling concurrent sysfs and debugfs reads?

In drivers/hwmon/corsair-psu.c, the shared USB command buffer
(priv->cmd_buffer) and wait_completion state are mutated in
corsairpsu_usb_cmd() without thread synchronization. While sysfs accesses
are protected by the hwmon core lock, debugfs callbacks like print_uptime()
funnel through to the USB request path without acquiring any locks.

If a user issues concurrent reads to hwmon sysfs attributes and debugfs
attributes, could this mutate the shared priv->cmd_buffer and
priv->wait_completion simultaneously, potentially leading to data corruption
or deadlocks via corrupted wait_completion state?

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