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

[sashiko-bot@kernel.org]

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