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

[Guenter Roeck]

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.

Guenter

---
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.                                                           
  • 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.                                                                                                                                                            
  
  ### 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 .
  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.                                                                                                
  
  ### 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();
    
  ### 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 ).