[PATCH] hwmon: (ina2xx) Fix overflow issues

[PATCH] hwmon: (ina2xx) Fix overflow issues

[Guenter Roeck]

Sashiko reports the following overflow problems:

In ina2xx_get_value(), the INA2XX_POWER calculation is:
    val = regval * data->power_lsb_uW;
The result is returned as a signed 32-bit int. For the INA232 with a common
2mOhm shunt, power_lsb_uW becomes 40,000. When the 16-bit regval exceeds
53,687, the product exceeds INT_MAX. This overflows the 32-bit signed math,
wrapping to a negative integer and reporting erroneous negative power
readings to userspace.

For INA2XX_POWER, the upper bound is clamped:
    val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
clamp_val() implicitly casts the upper bound to a 32-bit signed long on
32-bit platforms. This results in a negative number, meaning any valid
power limit is clamped to a negative value and ultimately programmed as 0.
Similarly, for INA2XX_SHUNT_VOLTAGE, the initial clamp uses SHRT_MAX *
shunt_div instead of division. The subsequent multiplication:
    val *= data->config->shunt_div;
overflows LONG_MAX on 32-bit platforms for high inputs, wrapping to a
negative value and also programming the hardware limit to 0.

For INA2XX_BUS_VOLTAGE on parts with bus_voltage_shift > 0, the calculation
(val * 1000) << shift can exceed LONG_MAX for limits over ~134V, wrapping
to negative and setting the limit to 0.

Have ina2xx_get_value() return a long variable to improve the supported
value range on 64-bit systems and to match the type os values returned
to the hwmon core.

Clamp the result of 'regval * data->power_lsb_uW' to LONG_MAX to fix the
INA2XX_POWER calculation overflow.

Change the initial clamp for INA2XX_SHUNT_VOLTAGE to SHRT_MAX / shunt_div
to fix the shunt limit overflow.

For INA2XX_BUS_VOLTAGE, limit the initial clamp to 130V instead of 200V
to avoid the overflow.

Cc: Loic Poulain <loic.poulain@oss.qualcomm.com>
Fixes: ab7fbee452be ("hwmon: (ina2xx) Fix various overflow issues")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ina2xx.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index c4742e84b999..0615666e430e 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -16,6 +16,7 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/limits.h>
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
@@ -266,10 +267,10 @@ static u16 ina226_interval_to_reg(long interval)
 	return FIELD_PREP(INA226_AVG_RD_MASK, avg_bits);
 }
 
-static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
-			    unsigned int regval)
+static long ina2xx_get_value(struct ina2xx_data *data, u8 reg,
+			     unsigned int regval)
 {
-	int val;
+	long val;
 
 	switch (reg) {
 	case INA2XX_SHUNT_VOLTAGE:
@@ -283,7 +284,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
 		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_POWER:
-		val = regval * data->power_lsb_uW;
+		val = clamp_val((u64)regval * data->power_lsb_uW, 0, LONG_MAX);
 		break;
 	case INA2XX_CURRENT:
 		/* signed register, result in mA */
@@ -380,17 +381,17 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
 {
 	switch (reg) {
 	case INA2XX_SHUNT_VOLTAGE:
-		val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
+		val = clamp_val(val, 0, DIV_ROUND_CLOSEST(SHRT_MAX, data->config->shunt_div));
 		val *= data->config->shunt_div;
 		val <<= data->config->shunt_voltage_shift;
 		return clamp_val(val, 0, SHRT_MAX);
 	case INA2XX_BUS_VOLTAGE:
-		val = clamp_val(val, 0, 200000);
+		val = clamp_val(val, 0, 130000);
 		val = (val * 1000) << data->config->bus_voltage_shift;
 		val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
 		return clamp_val(val, 0, USHRT_MAX);
 	case INA2XX_POWER:
-		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
+		val = clamp_val(val, 0, LONG_MAX - data->power_lsb_uW);
 		val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
 		return clamp_val(val, 0, USHRT_MAX);
 	case INA2XX_CURRENT:
-- 
2.45.2

Re: [PATCH] hwmon: (ina2xx) Fix overflow issues

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing integer overflow in INA2XX_CURRENT read calculation on 32-bit platforms.
- [High] Pre-existing integer overflow in INA2XX_CURRENT alert limit conversion on 32-bit platforms.
- [High] Pre-existing integer overflow in sy24655_average_power_read calculation on 32-bit platforms.
--

commit 9e5ee5483a3c4bc84b89199266642adb73bf1c08
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (ina2xx) Fix overflow issues

This commit fixes multiple integer overflow issues in the ina2xx driver
that occurred on 32-bit platforms. It addresses incorrect clamping and
signed 32-bit arithmetic overflows when calculating power, shunt voltage,
and bus voltage, and updates the return type of ina2xx_get_value() to long.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 613ffb622b7c4..12ae874d0e197 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -283,7 +284,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
>  		val = DIV_ROUND_CLOSEST(val, 1000);
>  		break;
>  	case INA2XX_POWER:
> -		val = regval * data->power_lsb_uW;
> +		val = clamp_val((u64)regval * data->power_lsb_uW, 0, LONG_MAX);
>  		break;
>  	case INA2XX_CURRENT:
>  		/* signed register, result in mA */
>  		val = ((s16)regval >> data->config->current_shift) *
>  		  data->current_lsb_uA;

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation overflow
on 32-bit platforms?

On 32-bit systems, long is 32-bit signed. For configurations with small
shunts, current_lsb_uA can be very large (e.g., 40,000,000). When multiplied
by a large regval, the product can reach 1.3 trillion, massively exceeding
LONG_MAX (2.14 billion).

This will silently overflow the 32-bit signed math, wrapping to a negative
value, which causes the subsystem to report radically incorrect (negative)
currents, potentially causing automated power capping or thermal safety scripts
to fail to throttle the system under load.

>  		val = DIV_ROUND_CLOSEST(val, 1000);
>  		break;

[ ... ]

> @@ -380,17 +381,17 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
>  {
>  	switch (reg) {

[ ... ]

>  	case INA2XX_BUS_VOLTAGE:
> -		val = clamp_val(val, 0, 200000);
> +		val = clamp_val(val, 0, 130000);
>  		val = (val * 1000) << data->config->bus_voltage_shift;
>  		val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
>  		return clamp_val(val, 0, USHRT_MAX);
>  	case INA2XX_POWER:
> -		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
> +		val = clamp_val(val, 0, LONG_MAX - data->power_lsb_uW);
>  		val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
>  		return clamp_val(val, 0, USHRT_MAX);
>  	case INA2XX_CURRENT:
>  		val = clamp_val(val, INT_MIN / 1000, INT_MAX / 1000);
>  		/* signed register, result in mA */
>  		val = DIV_ROUND_CLOSEST(val * 1000, data->current_lsb_uA);
>  		val <<= data->config->current_shift;

[Severity: High]
This is a pre-existing issue, but can this left-shift silently overflow the
32-bit signed long before the final hardware clamp?

If a large limit is provided to effectively disable the alert (e.g.,
INT_MAX / 1000), the intermediate value after DIV_ROUND_CLOSEST can be large
(e.g., 200,000,000). When left-shifted by current_shift (e.g., 4 for INA234),
it becomes 3.2 billion, exceeding LONG_MAX and wrapping to a negative number.

The subsequent clamp_val() incorrectly clamps this negative value to
SHRT_MIN (-32768) rather than saturating at the maximum positive limit, which
would program a permanent false alert into the hardware and cause continuous
spurious interrupts that can trigger unwanted system shutdowns.

>  		return clamp_val(val, SHRT_MIN, SHRT_MAX);
>  	default:

I also noticed a pre-existing issue in sy24655_average_power_read()
where a similar multiplication overflow occurs on 32-bit systems:

drivers/hwmon/ina2xx.c:sy24655_average_power_read() {
    ...
	if (sample_count <= 0) {
		*val = 0;
		return 0;
	}

	*val = DIV_ROUND_CLOSEST(accumulator_24, sample_count) * data->power_lsb_uW;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the accumulator quotient
multiplied by power_lsb_uW overflow the 32-bit signed math before
assignment?

The accumulator can reach 16,777,215. When multiplied by a typical
power_lsb_uW (e.g., 25,000), the product reaches ~419 billion, far
exceeding LONG_MAX (2.14 billion).

Because average power readings are critical for thermal and power management,
returning invalid or wrapped negative data could cause the system to fail to
throttle under heavy load.

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