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

[Thomas Weißschuh]

On 2026-06-30 16:06:04-0700, Guenter Roeck wrote:
> On Tue, Jun 30, 2026 at 10:57:52PM +0200, Thomas Weißschuh wrote:
> > The calculations converting between the different temperature units can
> > overflow, resulting in incorrect data.
> > 
> > Detect these overflows and report them.
> > 
> 
> I did not see how this can happen in practice, so I asked an AI agent.
> Its response is below.
>
> Given that, I am not going to apply this patch. I am all for checking
> against overflows, but only if they are real.

They can happen on 32-bit architectures. 
So my idea was to have a single shared algorithm for which the compiler
will nicely optimize away the impossible cases.

(More below)

> ---
> Yes, an overflow is possible, but only on 32-bit architectures.
>   
>   Here is the breakdown of why this happens and its limitations:
>   
>   ### 1.  cros_ec_hwmon_read_temp_threshold 
>   
>   The function  cros_ec_hwmon_read_temp_threshold  populates the  threshold  variable using  resp.temp_host[threshold] , which is an array of  uint32_t .
>   
>   In  cros_ec_hwmon_read , this  u32  threshold is passed to the conversion function which takes a  long t :
>   
>     static bool cros_ec_hwmon_kelvin_to_millicelsius_overflow(long t, long *ret)
>     
>   • On a 32-bit system:  long  is a 32-bit signed integer with a
>   maximum value of  2,147,483,647 . If the EC hardware returns a bogus
>   or uninitialized positive  u32  value (e.g.,  3,000,000 ), the
>   multiplication  t * MILLIDEGREE_PER_DEGREE  ( 3,000,000 * 1000 =
>   3,000,000,000 ) will exceed  LONG_MAX  and cause a signed integer
>   overflow.                                                           

This is the case I want to handle.

>   • On a 64-bit system:  long  is a 64-bit integer. The maximum
>   possible  u32  value multiplied by  1000  is  4,294,967,295,000 ,
>   which comfortably fits within the 64-bit limit of  9.22 x 10^18 . An
>   overflow here is mathematically impossible.                                                                                                                                                            
And the compiler knows this and optimizes away the overflow checks.

>   
>   ### 2. A Quirky Edge Case on 32-bit
>   
>   If the EC returns a standard error state like  0xFFFFFFFF , the
>   implicit cast from  u32 threshold  to  long t  on a 32-bit system
>   will result in  t = -1 .

This "standard error" state does not apply to these values.
'0' means 'disabled', all other values are normal values.

>   Interestingly, the new  check_mul_overflow(-1, 1000, ret)  check
>   will not trigger, because  -1000  fits perfectly inside a 32-bit
>   long . Instead, it will silently return a bogus value of  -274000 
>   (which corresponds to -274°C). The patch does not actually prevent
>   this specific "incorrect data" case.                                                                                                

This is a bug in my code. I'd like to fix it like below, so the compiler
can optimize this away on 64-bit, too.

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

>   ### 3.  cros_ec_hwmon_temp_to_millicelsius 
>   
>   For the standard temperature readings (where  temp  is a  u8 ), an
>   overflow is impossible on all architectures.
>   The maximum value of  u8  is 255. When the offset of 200 is added,
>   the maximum base is 455. Multiplying  455 * 1000  is  455,000 ,
>   which easily fits in a 32-bit (and even 16-bit) integer. The patch
>   author is clearly aware of this, which is why the patch asserts its
>   impossibility via  BUILD_BUG() :                                                                                                   
>   
>         if (check_add_overflow(temp, EC_TEMP_SENSOR_OFFSET, &ret))
>             BUILD_BUG();
>     
>         if (cros_ec_hwmon_kelvin_to_millicelsius_overflow(ret, &ret))
>             BUILD_BUG();

That is correct. The idea is again to share one conversion function
between all users to avoid code duplication.

>   ### Conclusion
>   
>   The patch's claim that calculations "can overflow" is accurate, but
>   the scope of the fix is limited strictly to threshold registers
>   running on 32-bit systems, and it will still emit bogus negative
>   temperatures for unsigned integer values larger than  LONG_MAX
>   (such as  0xFFFFFFFF ).

I'll also mention that the overflow only affects 32-bit systems
and that the compiler will optimize away the checks on 64-bit.

Does that make sense?


Thomas