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

[sashiko-bot@kernel.org]

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