Messages in this thread Patch in this message |  | | Date | Tue, 7 Apr 2026 20:21:46 +0100 | | From | David Laight <> | | Subject | Re: [PATCH v2 2/3] hwmon: (isl28022) Fix integer overflow in power calculation on 32-bit |
| |
On Tue, 7 Apr 2026 17:38:01 +0000 "Pradhan, Sanman" <sanman.pradhan@hpe.com> wrote:
> From: Sanman Pradhan <psanman@juniper.net> > > isl28022_read_power() computes: > > *val = ((51200000L * ((long)data->gain)) / > (long)data->shunt) * (long)regval; > > On 32-bit platforms, 'long' is 32 bits. With gain=8 and shunt=10000 > (the default configuration): > > (51200000 * 8) / 10000 = 40960 > 40960 * 65535 = 2,684,313,600 > > This exceeds LONG_MAX (2,147,483,647), resulting in signed integer > overflow. > > Additionally, dividing before multiplying by regval loses precision > unnecessarily. > > Use u64 intermediates with div_u64() and multiply before dividing > to retain precision. Power is inherently non-negative, so unsigned > types are the natural fit. Clamp the result to LONG_MAX before > returning it through the hwmon callback, following the pattern used > by ina238. > > Fixes: 39671a14df4f2 ("hwmon: (isl28022) new driver for ISL28022 power monitor") > Cc: stable@vger.kernel.org > Signed-off-by: Sanman Pradhan <psanman@juniper.net> > --- > v2: > - Switch from s64/div_s64() to u64/div_u64() since power is > inherently non-negative, avoiding implicit u32-to-s32 narrowing > of the shunt divisor
There is no such thing as u32-to-s32 narrowing. Basically nothing happens to the bit pattern. But the values are almost certainly never negative.
> > drivers/hwmon/isl28022.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c > index c2e559dde63f..d233a7b3f327 100644 > --- a/drivers/hwmon/isl28022.c > +++ b/drivers/hwmon/isl28022.c > @@ -9,6 +9,7 @@ > #include <linux/err.h> > #include <linux/hwmon.h> > #include <linux/i2c.h> > +#include <linux/math64.h> > #include <linux/module.h> > #include <linux/regmap.h> > > @@ -178,6 +179,7 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val) > struct isl28022_data *data = dev_get_drvdata(dev); > unsigned int regval; > int err; > + u64 tmp; > > switch (attr) { > case hwmon_power_input: > @@ -185,8 +187,9 @@ static int isl28022_read_power(struct device *dev, u32 attr, long *val) > ISL28022_REG_POWER, ®val); > if (err < 0) > return err; > - *val = ((51200000L * ((long)data->gain)) / > - (long)data->shunt) * (long)regval; > + tmp = (u64)51200000 * data->gain * regval; > + tmp = div_u64(tmp, data->shunt); > + *val = clamp_val(tmp, 0, LONG_MAX);
Don't use clamp_val(), you don't need to and it is completely broken by design. Just use min(). You could just write: *val = min(div_u64(51200000ULL * data->gain * regval, data->shunt), LONG_MAX);
Have you checked that the multiply can't overflow 64bits? That might be why the last multiply was done after the divide.
David
> break; > default: > return -EOPNOTSUPP;
|  |