Re: [PATCH] hwmon: it87: Reject negative values in set_fan()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Returning -EINVAL for out-of-range negative values breaks established hwmon ABI behavior, which expects out-of-bounds inputs to be clamped.

Pre-existing issues:
- [High] The patch fails to prevent signed integer overflows in `FAN16_TO_REG()` for large positive inputs, resulting in incorrectly calculated fan speed limits.
- [High] Other sysfs store functions (`set_in` and `set_temp`) lack upper boundary checks, leading to signed integer overflows and logically incorrect limits.
--

commit f17137aa0859283c1c9cba428192374fa60038f1
Author: Nikita Zhandarovich <n.zhandarovich@fintech.ru>

hwmon: it87: Reject negative values in set_fan()

This patch rejects negative values inputted to set_fan() in the it87 driver
by returning -EINVAL. It aims to prevent invalid arithmetic and undefined
behavior from signed overflow in the register conversion path.

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 5fd310662ee43..888db19758763 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)
> +		return -EINVAL;
> +

[Severity: Medium]
Does rejecting negative values with -EINVAL break backward compatibility
with userspace tools?

The hwmon subsystem documentation dictates that out-of-range inputs for
continuous limit attributes (like fan limits) should be clamped to their
minimum or maximum boundaries.

Would it be better to clamp the input here instead of abruptly returning
-EINVAL?

>  	err = it87_lock(data);
>  	if (err)
>  		return err;

[Severity: High]
This is a pre-existing issue, but does FAN16_TO_REG() still suffer from
signed integer overflows for excessively large positive inputs further down
in this function?

On 32-bit systems, if val is an extremely large positive RPM (e.g.,
2,147,000,000), the calculations in FAN16_TO_REG() can overflow to negative
values. The subsequent division could yield a positive value that is then
clamped and written to the register, resulting in a very low RPM limit
instead of capping at the maximum limit.

Could we clamp val to a safe upper bound before the register conversion
to prevent these overflows?

[Severity: High]
This isn't a bug introduced by this patch, but do the sysfs store functions
set_in() and set_temp() have similar signed integer overflow issues due to
missing upper boundary checks?

In set_in(), reading a large unsigned value into a long can cause
val * 10 to overflow to a negative value on 32-bit systems. After division,
it clamps to 0 instead of the maximum limit of 255.

Similarly, in set_temp(), a large positive input causes val + 500 to overflow
to a negative value, which is then divided and clamped to -128 instead of 127.

Should boundary checks or clamping be added to these functions as well to
ensure safe arithmetic and logically correct limits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529121141.1633588-1-n.zhandarovich@fintech.ru?part=1