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

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

[Thomas Weißschuh]

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;
 }
 
 static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
 {
-	return cros_ec_hwmon_kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
+	long ret;
+
+	if (check_add_overflow(temp, EC_TEMP_SENSOR_OFFSET, &ret))
+		BUILD_BUG();
+
+	if (cros_ec_hwmon_kelvin_to_millicelsius_overflow(ret, &ret))
+		BUILD_BUG();
+
+	return ret;
 }
 
 static bool cros_ec_hwmon_attr_is_temp_threshold(u32 attr)
@@ -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;
+			}
 		}
 	}
 

---
base-commit: 0109905cfdf4b64da014596f31c2ee5029ef0870
change-id: 20260630-cros_ec-hwmon-overflow-0381c8509df3

Best regards,
--  
Thomas Weißschuh <linux@weissschuh.net>

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

[Guenter Roeck]

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 ?

> +	long ret;
> +
> +	if (check_add_overflow(temp, EC_TEMP_SENSOR_OFFSET, &ret))
> +		BUILD_BUG();
> +
> +	if (cros_ec_hwmon_kelvin_to_millicelsius_overflow(ret, &ret))
> +		BUILD_BUG();
> +
> +	return ret;
>   }
>   
>   static bool cros_ec_hwmon_attr_is_temp_threshold(u32 attr)
> @@ -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;
> +			}

I agree with Sashiko - it does not make sense to call cros_ec_hwmon_kelvin_to_millicelsius_overflow()
if it is already known that threshold overflowed.

Personally I think this is way too complicated. An overflowing temperature above 255 degrees C
does not make any sense, and returning LONG_MAX as temperature seems a bit odd.
I would just have checked for some reasonable limit, such as
			if (threshold > 255 + 273)
				*val = 255000;
			else
				*val = cros_ec_hwmon_kelvin_to_millicelsius(threshold);

If it does make sense to return LONG_MAX as temperature for some reason, that should
be explained.

Thanks,
Guenter

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

[Thomas Weißschuh]

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.

> > +	long ret;
> > +
> > +	if (check_add_overflow(temp, EC_TEMP_SENSOR_OFFSET, &ret))
> > +		BUILD_BUG();
> > +
> > +	if (cros_ec_hwmon_kelvin_to_millicelsius_overflow(ret, &ret))
> > +		BUILD_BUG();
> > +
> > +	return ret;
> >   }
> >   static bool cros_ec_hwmon_attr_is_temp_threshold(u32 attr)
> > @@ -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;
> > +			}
> 
> I agree with Sashiko - it does not make sense to call cros_ec_hwmon_kelvin_to_millicelsius_overflow()
> if it is already known that threshold overflowed.

Yep, this is a bug. This would be correct:

if (ret == 0) {
       if (overflows_type(threshold, long) ||
	   cros_ec_hwmon_kelvin_to_millicelsius_overflow(threshold, val))
	       *val = LONG_MAX;
}

> Personally I think this is way too complicated. An overflowing temperature above 255 degrees C
> does not make any sense, and returning LONG_MAX as temperature seems a bit odd.
> I would just have checked for some reasonable limit, such as
> 			if (threshold > 255 + 273)
> 				*val = 255000;
> 			else
> 				*val = cros_ec_hwmon_kelvin_to_millicelsius(threshold);

That works for me, too. Thanks.

> If it does make sense to return LONG_MAX as temperature for some reason, that should
> be explained.

No specific reason.


Thomas

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

[Guenter Roeck]

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