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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix for integer overflow in `ina226_alert_to_reg` for `INA2XX_CURRENT`.
--

commit 66f6bbb6849145fb218d70b58635416ea1a7ba0d
Author: Guenter Roeck <linux@roeck-us.net>

    hwmon: (ina2xx) Fix overflow issues

    This commit addresses several integer overflow problems in the ina2xx
    driver by introducing various value clamps and switching to larger data
    types. Specifically, it attempts to fix overflows in ina2xx_get_value()
    and ina226_alert_to_reg().

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index c4742e84b999d..8a219514b03be 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
[ ... ]
> @@ -378,23 +381,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_CURRENT:
> -		val = clamp_val(val, INT_MIN / 1000, INT_MAX / 1000);
> +		limit = (LONG_MAX - (data->current_lsb_uA / 2)) / 1000;
> +		val = clamp_val(val, -limit, limit);

[Severity: High]
This is a pre-existing issue, but does this initial clamp properly account
for the current_shift?

The commit message explicitly states: "To address the INA2XX_CURRENT
problem in ina226_alert_to_reg(), take current_shift into acount for the
initial clamp to avoid the overflow."

However, data->config->current_shift doesn't appear to be used in the
calculation of the limit here.

>  		/* signed register, result in mA */
>  		val = DIV_ROUND_CLOSEST(val * 1000, data->current_lsb_uA);
>  		val <<= data->config->current_shift;

If a user writes a very large limit on a 32-bit platform to disable the
alert (e.g., INT_MAX), val is clamped to approximately 2,147,483.
val * 1000 evaluates to 2,147,483,000.

After DIV_ROUND_CLOSEST with a small current_lsb_uA (e.g., 10 for a
typical 0.2-ohm shunt), val becomes 214,748,300.

When left-shifting by current_shift (e.g., 4 for the INA234), val becomes
3,435,972,800. Since val is a 32-bit signed long, this exceeds LONG_MAX
and wraps around to -858,994,496.

This negative value will then be clamped to SHRT_MIN in the return
statement, programming the hardware to immediately trigger false alerts
when the user's intent was to disable them.

Should current_shift be included in the initial clamp calculation to
prevent this?

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

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