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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The INA2XX_CURRENT limit calculation artificially restricts physically valid high-current alerts due to a mathematically flawed clamp derivation.
--

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

hwmon: (ina2xx) Fix overflow issues

This patch updates the return type of ina2xx_get_value() to long to
support a wider range of values on 64-bit systems. It also adds and adjusts
clamping limits in ina2xx_get_value() and ina226_alert_to_reg() to prevent
integer overflows during intermediate calculations.

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

[ ... ]

> @@ -378,23 +381,27 @@ 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 = ((INT_MAX - data->current_lsb_uA) / 1000) >>
> +		  data->config->current_shift;
> +		val = clamp_val(val, -limit, limit);

[Severity: High]
Does this calculation miscalculate the maximum safe input limit by not
accounting for the subsequent division?

To prevent the final shifted value (val * 1000 / current_lsb_uA) << shift
from overflowing, the limit would need to factor in the division by
current_lsb_uA. Instead, this formula subtracts the LSB and effectively
shifts the INT_MAX / 1000 limit right by current_shift.

For devices like the INA234 (current_shift = 4) configured with a small
shunt, the LSB can be very large. As noted in the commit message,
current_lsb_uA can reach 40,000,000. In this configuration, the hardware's
physical maximum alert limit is (32767 >> 4) * 40A = 81,880A.

By not multiplying the allowable upper bound by the LSB before the shift,
does this artificially cap the user input to ((INT_MAX - 40000000) / 1000)
>> 4 = 131,717 mA (131.7A)? This restricts users to less than 0.2% of the
sensor's valid measurement range.

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

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