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

[PATCH v2] 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.

For INA2XX_CURRENT:
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.

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.

In sy24655_average_power_read(), the accumulator quotient multiplied by
power_lsb_uW can overflow the 32-bit signed math before the assignment.

Have ina2xx_get_value() return a long variable to improve the supported
value range on 64-bit systems and to match the type of 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.

For INA2XX_CURRENT, improve clamping to avoid the overflow.

To address the INA2XX_CURRENT problem in ina226_alert_to_reg(), take
current_shift into acount for the initial clamp to avoid the overflow.

In sy24655_average_power_read, use a temporary 64-bit variable to store
the multiplication result and clamp the result against LONG_MAX.

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>
---
v2: Fixed several additional overflow conditions reported by Sashiko
    after v1

 drivers/hwmon/ina2xx.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index c4742e84b999..1ba46c9c3e20 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,11 @@ 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;
+	s64 val64;
+	long val;
 
 	switch (reg) {
 	case INA2XX_SHUNT_VOLTAGE:
@@ -283,13 +285,13 @@ 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) *
+		val64 = (s64)((s16)regval >> data->config->current_shift) *
 		  data->current_lsb_uA;
-		val = DIV_ROUND_CLOSEST(val, 1000);
+		val = clamp_val(DIV_ROUND_CLOSEST(val64, 1000), LONG_MIN, LONG_MAX);
 		break;
 	case INA2XX_CALIBRATION:
 		val = regval;
@@ -378,23 +380,26 @@ static int ina2xx_read_init(struct device *dev, int reg, long *val)
  */
 static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
 {
+	long limit;
+
 	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:
-		val = clamp_val(val, INT_MIN / 1000, INT_MAX / 1000);
+		limit = (INT_MAX / 1000) >> data->config->current_shift;
+		val = clamp_val(val, -limit, limit);
 		/* signed register, result in mA */
 		val = DIV_ROUND_CLOSEST(val * 1000, data->current_lsb_uA);
 		val <<= data->config->current_shift;
@@ -537,6 +542,7 @@ static int sy24655_average_power_read(struct ina2xx_data *data, u8 reg, long *va
 	u8 template[6];
 	int ret;
 	long accumulator_24, sample_count;
+	u64 val64;
 
 	/* 48-bit register read */
 	ret = i2c_smbus_read_i2c_block_data(data->client, reg, 6, template);
@@ -555,7 +561,8 @@ static int sy24655_average_power_read(struct ina2xx_data *data, u8 reg, long *va
 		return 0;
 	}
 
-	*val = DIV_ROUND_CLOSEST(accumulator_24, sample_count) * data->power_lsb_uW;
+	val64 = (u64)DIV_ROUND_CLOSEST(accumulator_24, sample_count) * data->power_lsb_uW;
+	*val = clamp_val(val64, 0, LONG_MAX);
 
 	return 0;
 }
-- 
2.45.2

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

[sashiko-bot]

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

New issues:
- [Low] Build breakage: Using `DIV_ROUND_CLOSEST()` with an `s64` dividend causes a linker error (`undefined reference to '__divdi3'`) on 32-bit platforms.

Pre-existing issues:
- [High] Arithmetic overflow in `ina226_alert_to_reg()` during `DIV_ROUND_CLOSEST` intermediate calculation on 32-bit platforms.
--

commit 2efec75618467bc75c51301a14034dc4018a9013
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (ina2xx) Fix overflow issues

This patch aims to fix multiple overflow issues in the ina2xx driver,
specifically when calculating power, voltage, and current values on 32-bit
platforms. It introduces 64-bit variables and clamped limits to prevent
numbers from wrapping around.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 613ffb622b7c4..b2f543b89f9e8 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c

[ ... ]

> @@ -283,13 +285,13 @@ 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) *
> +		val64 = (s64)((s16)regval >> data->config->current_shift) *
>  		  data->current_lsb_uA;
> -		val = DIV_ROUND_CLOSEST(val, 1000);
> +		val = clamp_val(DIV_ROUND_CLOSEST(val64, 1000), LONG_MIN, LONG_MAX);

[Severity: Low]
Does this cause a build failure on 32-bit platforms?

The variable val64 is an s64, and DIV_ROUND_CLOSEST relies on the native
division operator. Doing a 64-bit signed division by a non-power-of-2
constant like 1000 typically causes a linker error (undefined reference to
__divdi3) because the kernel does not link against libgcc.

Should this use div_s64() instead?

[ ... ]

> @@ -378,23 +380,26 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
>  	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);
> +		limit = (INT_MAX / 1000) >> data->config->current_shift;
> +		val = clamp_val(val, -limit, limit);
>  		/* signed register, result in mA */
>  		val = DIV_ROUND_CLOSEST(val * 1000, data->current_lsb_uA);

[Severity: High]
This is a pre-existing issue, but can the intermediate calculation inside
DIV_ROUND_CLOSEST overflow the 32-bit signed integer?

The macro internally performs:
(val * 1000) + (data->current_lsb_uA / 2)

On 32-bit systems where long is 32-bit, val * 1000 can be up to
2,147,483,000. If data->current_lsb_uA is large (e.g., 40,000,000 as noted
in the commit message), adding current_lsb_uA / 2 (20,000,000) yields
2,167,483,000.

This exceeds LONG_MAX (2,147,483,647) and silently wraps to a negative
value. Will this result in a large positive limit requested by the user
being incorrectly programmed as a negative limit in hardware?

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