Re: [PATCH v2] hwmon: (cros_ec) Handle temperature conversion overflows

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `else` statement causes clamped threshold values to be incorrectly overwritten by the fallback calculation on 32-bit systems.
--

--- Patch [1]: [PATCH v2] hwmon: (cros_ec) Handle temperature conversion overflows ---
commit 89530e9a6ba047e5c308ed9d02243a168afc676c
Author: Thomas Weißschuh <[email protected]>

hwmon: (cros_ec) Handle temperature conversion overflows

This commit detects potential arithmetic overflows when converting between
different temperature units on 32-bit systems and aims to clamp the returned
values to avoid reporting incorrect data.

> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 1337b646e0228..004a8180b665e 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c

[ ... ]

> @@ -236,8 +252,13 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			ret = cros_ec_hwmon_read_temp_threshold(priv->cros_ec, channel,
>  								cros_ec_hwmon_attr_to_thres(attr),
>  								&threshold);
> -			if (ret == 0)
> -				*val = cros_ec_hwmon_kelvin_to_millicelsius(threshold);
> +			if (ret == 0) {
> +				if (overflows_type(threshold, long))
> +					*val = LONG_MAX;
> +
> +				if (cros_ec_hwmon_kelvin_to_millicelsius_overflow(threshold, val))
> +					*val = LONG_MAX;

[Severity: High]
Does the lack of an else statement here cause the clamped LONG_MAX value to
be overwritten?

If the threshold is a large u32 value (e.g., 0xFFFFFFFF) on a 32-bit system,
overflows_type() returns true and sets *val to LONG_MAX. However, because
there is no else clause before the next check,
cros_ec_hwmon_kelvin_to_millicelsius_overflow() is unconditionally evaluated.

Since threshold is implicitly cast to long when passed into the function, the
large u32 value can become a small negative number (like -1) on 32-bit
architectures. The subsequent arithmetic inside the function would not
overflow, and the out-parameter assignment overwrites the LONG_MAX in *val
with an incorrect negative value (such as -274000).

> +			}
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1