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

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

[Nikita Zhandarovich]

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.

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;

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

[Guenter Roeck]

On Fri, May 29, 2026 at 05:18:36PM +0300, Nikita Zhandarovich 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.
> 
> 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>

Applied.

Thanks,
Guenter

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

[David Laight]

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;
>