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

[Guenter Roeck <linux@roeck-us.net>]

On 7/29/26 08:40, Thomas Weißschuh wrote:
> On 2026-07-28 08:02:28-0700, Guenter Roeck wrote:
>> On 7/28/26 03:17, Thomas Weißschuh wrote:
>>> The calculations converting between the different temperature units can
>>> overflow on 32-bit systems, resulting in incorrect data.
>>>
>>> Detect these overflows and handle them.
>>>
>>> The code is written in a way that there is a single conversion function
>>> and the compiler can recognize when overflows are impossible (on 64-bit
>>> and in cros_ec_hwmon_temp_to_millicelsius()). If the compiler detects
>>> this, it will optimize away the overflow checks.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>> Changes in v2:
>>> - Drop already applied patch 1.
>>> - Also handle overflow of u32 -> long.
>>> - Clarify commit message wrt compiler optimizations.
>>> - Use __always_inline over __flatten to allow the compiler to optimize
>>>     away more unnecessary overflow checks.
>>> - Link to v1: https://patch.msgid.link/20260630-cros_ec-hwmon-overflow-v1-0-3d2ecd3eb0f2@weissschuh.net
>>> ---
>>>    drivers/hwmon/cros_ec_hwmon.c | 31 ++++++++++++++++++++++++++-----
>>>    1 file changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>>> index 1337b646e022..004a8180b665 100644
>>> --- a/drivers/hwmon/cros_ec_hwmon.c
>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>> @@ -5,11 +5,13 @@
>>>     *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
>>>     */
>>> +#include <linux/build_bug.h>
>>>    #include <linux/cleanup.h>
>>>    #include <linux/device.h>
>>>    #include <linux/hwmon.h>
>>>    #include <linux/math.h>
>>>    #include <linux/module.h>
>>> +#include <linux/overflow.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/platform_data/cros_ec_commands.h>
>>>    #include <linux/platform_data/cros_ec_proto.h>
>>> @@ -151,14 +153,28 @@ static bool cros_ec_hwmon_is_error_temp(u8 temp)
>>>    /* This differs slightly from the variant in units.h to avoid rounding inconsistencies. */
>>>    #define CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS (-273000)
>>> -static long cros_ec_hwmon_kelvin_to_millicelsius(long t)
>>> +static __always_inline bool cros_ec_hwmon_kelvin_to_millicelsius_overflow(long t, long *ret)
>>>    {
>>> -	return t * MILLIDEGREE_PER_DEGREE + CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS;
>>> +	if (check_mul_overflow(t, MILLIDEGREE_PER_DEGREE, ret))
>>> +		return true;
>>> +
>>> +	if (check_add_overflow(*ret, CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS, ret))
>>> +		return true;
>>> +
>>> +	return false;
>>
>> 	return check_add_overflow(*ret, CROS_EC_HWMON_ABSOLUTE_ZERO_MILLICELSIUS, ret);
>>>    }
>>>    static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
>>
>> The maximum value of "temp" is 255.
>>
>>>    {
>>> -	return cros_ec_hwmon_kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
>>
>> 255 + 200 = 455.
>> 455 * 1000 = 455000 (not even counting the conversion from kelvin to C).
>>
>> How could this ever overflow ?
> 
> It will never overflow. And the compiler knows this and optimizes away
> all the overflow checks. Otherwise the build would straight up fail to
> the BUILD_BUG()s below.
> 
> The idea was that not everyone reading this code needs to do the
> calculations and double-check.
> 

No, sorry. You accomplished the opposite: You made me re-check the calculations
because it was obvious to me that there can be no overflow. So you wasted
my time. I am absolutely opposed to unnecessary checks, even if they are compiled
away.

Guenter