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

On 8/5/26 09:14, Nuno Sá wrote:
> 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.
> 
Makes sense. I'll apply the series as-is. If you (or me) ever get to
it we can always improve on it.

Thanks,
Guenter