Re: [PATCH 2/3] hwmon: (ltc4282) Clamp negative current limits

On Wed, Aug 05, 2026 at 08:41:30AM -0700, Guenter Roeck wrote:
> On 8/5/26 02:22, Nuno Sá wrote:
> > On Tue, Aug 04, 2026 at 05:57:20PM -0700, Guenter Roeck wrote:
> > > When a negative value is passed to ltc4282_write_curr(), the signed long
> > > val is cast directly to u64:
> > > 
> > > drivers/hwmon/ltc4282.c:ltc4282_write_curr() {
> > >          /* need to pass it in millivolt */
> > >          u32 in = DIV_ROUND_CLOSEST_ULL((u64)val * st->rsense, DECA * MICRO);
> > >          ...
> > > }
> > > 
> > > This cast converts negative inputs into large positive values. The
> > > subsequent division result overflows the u32 in variable, truncating
> > > to a pseudo-random positive value. When this is passed to
> > > ltc4282_write_voltage_byte(), it is clamped to the maximum limit instead
> > > of zero.
> > > 
> > > Clamp val to 0 and to the maximum supported upper limit before the cast
> > > and assign the result to a 64-bit temporary variable before the division
> > > to avoid the underflow and an also possible overflow.
> > > 
> > > Reported-by: Sashiko <[email protected]>
> > > Fixes: cbc29538dbf7d ("hwmon: Add driver for LTC4282")
> > > Cc: Nuno Sa <[email protected]>
> > > Signed-off-by: Guenter Roeck <[email protected]>
> > > ---
> > >   drivers/hwmon/ltc4282.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hwmon/ltc4282.c b/drivers/hwmon/ltc4282.c
> > > index bb7f6727c44d..bb1bcb369016 100644
> > > --- a/drivers/hwmon/ltc4282.c
> > > +++ b/drivers/hwmon/ltc4282.c
> > > @@ -14,6 +14,7 @@
> > >   #include <linux/hwmon.h>
> > >   #include <linux/i2c.h>
> > >   #include <linux/math.h>
> > > +#include <linux/math64.h>
> > >   #include <linux/minmax.h>
> > >   #include <linux/module.h>
> > >   #include <linux/regmap.h>
> > > @@ -929,8 +930,11 @@ static int ltc4282_curr_reset_hist(struct ltc4282_state *st)
> > >   static int ltc4282_write_curr(struct ltc4282_state *st, u32 attr,
> > >   			      long val)
> > >   {
> > > +	s32 ulimit = min_t(u64, INT_MAX,
> > > +			   div_u64((u64)INT_MAX * DECA * MICRO, st->rsense));
> > 
> > I guess we can do the same as in ltc4283 instead of just assuming
> > INT_MAX:
> > 
> > https://elixir.bootlin.com/linux/v7.2-rc5/source/drivers/hwmon/ltc4283.c#L765
> > 
> > And so we just account for isense_max
> > 
> 
> Sashiko isn't happy with it. It suggests the multiplier in the limit calculation
> should use MILLI, not MICRO, and that a 32-bit operation could overflow.
> With that, I'd rather keep it as is, or drop that patch and let you handle it.
> Please let me know what you prefer.
> 

I guess we would need to also do what we have in ltc4283 and limit the
range of values rsense is allowed to have so we can better reason on
what can or cannot overflow. But I can't really commit to when I can
handle this so I'm fine with the above approach.

- Nuno Sá