Re: [PATCH v2] hwmon: it87: Clamp negative values to zero in set_fan()

[David Laight <david.laight.linux@gmail.com>]

On Fri, 29 May 2026 17:18:36 +0300
Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:

> set_fan() parses user input with kstrtol() and passes the resulting
> value to FAN16_TO_REG() on chips with 16-bit fan support.
> 
> Negative fan speeds are not meaningful and should be rejected before
> conversion. Worst scenario, one may be able to abuse undefined
> behaviour of signed overflow to possibly induce rpm * 2 == 0 in
> FAN16_TO_REG(), thus causing a division by zero.

Wouldn't it be better to fix the bounds checks in FAN16_TO_REG()
and FAN_TO_REG()?
Both already treat 0 as special.

Oh - and just use clamp() not clamp_val().

-- David

> 
> Instead, clamp val < 0 to zero and keep the conversion in its valid
> input domain, avoiding unsafe arithmetic in the register conversion
> path.
> 
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
> 
> Fixes: 17d648bf5786 ("it87: Add support for the IT8716F")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> v2: as pointed out by sashiko-bot, returning with -EINVAL
> goes against hwmon guidelines - therefore, just clamp the lower
> bound and keep going. Commit description is adjusted accordingly,
> as is the subject text.
> 
> v1: https://lore.kernel.org/all/20260529121141.1633588-1-n.zhandarovich@fintech.ru/
> Sashiko AI review: https://lore.kernel.org/all/20260529125335.A47011F00893@smtp.kernel.org/
> 
> P.S. I've deliberately NOT addressed the pre-existing issues highlighted
> by sashiko - that's for another time, I think.
> 
>  drivers/hwmon/it87.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 5fd310662ee4..87edb1b6048b 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -1412,6 +1412,9 @@ static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
>  	if (kstrtol(buf, 10, &val) < 0)
>  		return -EINVAL;
>  
> +	if (val < 0)
> +		val = 0;
> +
>  	err = it87_lock(data);
>  	if (err)
>  		return err;
>