With an expiry value on the order of 20 hours, and with a bus scale value
of 256k (as supplied by qemu), the (signed) multiplication will be UB. As
we've checked that the value is positive, we mean unsigned multiplication
anyway. Yet let's play safe against even larger expiry and bus scale
values, leveraging the compiler builtin that there is for this purpose.

While there also drop the stray cast from the actual TMICT write.

Fixes: 9062553a0dc1 ("added time and accurate timer support")
Fixes: b95beb185810 ("x86: Clean up APIC local timer handling")
Reported-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Sadly neither gcc5 nor gcc15 properly optimize the (effectively) two uses
of the 0xffffffffU constant: Both use a 2nd register to load the constant
(really 0xfffffffeU unless <= is used) a 2nd time.

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1224,10 +1224,16 @@ int reprogram_timer(s_time_t timeout)
     }
 
     if ( timeout && ((expire = timeout - NOW()) > 0) )
-        apic_tmict = min_t(uint64_t, (bus_scale * expire) >> BUS_SCALE_SHIFT,
-                           UINT32_MAX);
+    {
+        unsigned long product;
 
-    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
+        apic_tmict = UINT32_MAX;
+        if ( !__builtin_umull_overflow(bus_scale, expire, &product) &&
+             (product >>= BUS_SCALE_SHIFT) < apic_tmict )
+            apic_tmict = product;
+    }
+
+    apic_write(APIC_TMICT, apic_tmict);
 
     return apic_tmict || !timeout;
 }

On 4/9/26 05:21, Jan Beulich wrote:
> With an expiry value on the order of 20 hours, and with a bus scale value

If I did my math correctly, I believe 10 hours is sufficient to trigger signed
multiplication overflow. 20 hours would result in unsigned multiplication
overflow.

> of 256k (as supplied by qemu), the (signed) multiplication will be UB. As
> we've checked that the value is positive, we mean unsigned multiplication
> anyway. Yet let's play safe against even larger expiry and bus scale
> values, leveraging the compiler builtin that there is for this purpose.
> 
> While there also drop the stray cast from the actual TMICT write.
> 
> Fixes: 9062553a0dc1 ("added time and accurate timer support")
> Fixes: b95beb185810 ("x86: Clean up APIC local timer handling")
> Reported-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Thanks!

On 09/04/2026 10:21 am, Jan Beulich wrote:
> With an expiry value on the order of 20 hours, and with a bus scale value
> of 256k (as supplied by qemu), the (signed) multiplication will be UB. As
> we've checked that the value is positive, we mean unsigned multiplication
> anyway. Yet let's play safe against even larger expiry and bus scale
> values, leveraging the compiler builtin that there is for this purpose.
>
> While there also drop the stray cast from the actual TMICT write.
>
> Fixes: 9062553a0dc1 ("added time and accurate timer support")
> Fixes: b95beb185810 ("x86: Clean up APIC local timer handling")
> Reported-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Sadly neither gcc5 nor gcc15 properly optimize the (effectively) two uses
> of the 0xffffffffU constant: Both use a 2nd register to load the constant
> (really 0xfffffffeU unless <= is used) a 2nd time.
>
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1224,10 +1224,16 @@ int reprogram_timer(s_time_t timeout)
>      }
>  
>      if ( timeout && ((expire = timeout - NOW()) > 0) )
> -        apic_tmict = min_t(uint64_t, (bus_scale * expire) >> BUS_SCALE_SHIFT,
> -                           UINT32_MAX);
> +    {
> +        unsigned long product;
>  
> -    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> +        apic_tmict = UINT32_MAX;
> +        if ( !__builtin_umull_overflow(bus_scale, expire, &product) &&
> +             (product >>= BUS_SCALE_SHIFT) < apic_tmict )
> +            apic_tmict = product;
> +    }
> +
> +    apic_write(APIC_TMICT, apic_tmict);
>  
>      return apic_tmict || !timeout;
>  }

This is fine for staging, but be aware it cannot be backported before
4.21 due to the toolchain baseline (and nothing in CI will notice, I
don't think.)

~Andrew

On 09.04.2026 11:39, Andrew Cooper wrote:
> On 09/04/2026 10:21 am, Jan Beulich wrote:
>> With an expiry value on the order of 20 hours, and with a bus scale value
>> of 256k (as supplied by qemu), the (signed) multiplication will be UB. As
>> we've checked that the value is positive, we mean unsigned multiplication
>> anyway. Yet let's play safe against even larger expiry and bus scale
>> values, leveraging the compiler builtin that there is for this purpose.
>>
>> While there also drop the stray cast from the actual TMICT write.
>>
>> Fixes: 9062553a0dc1 ("added time and accurate timer support")
>> Fixes: b95beb185810 ("x86: Clean up APIC local timer handling")
>> Reported-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1224,10 +1224,16 @@ int reprogram_timer(s_time_t timeout)
>>      }
>>  
>>      if ( timeout && ((expire = timeout - NOW()) > 0) )
>> -        apic_tmict = min_t(uint64_t, (bus_scale * expire) >> BUS_SCALE_SHIFT,
>> -                           UINT32_MAX);
>> +    {
>> +        unsigned long product;
>>  
>> -    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
>> +        apic_tmict = UINT32_MAX;
>> +        if ( !__builtin_umull_overflow(bus_scale, expire, &product) &&
>> +             (product >>= BUS_SCALE_SHIFT) < apic_tmict )
>> +            apic_tmict = product;
>> +    }
>> +
>> +    apic_write(APIC_TMICT, apic_tmict);
>>  
>>      return apic_tmict || !timeout;
>>  }
> 
> This is fine for staging, but be aware it cannot be backported before
> 4.21 due to the toolchain baseline (and nothing in CI will notice, I
> don't think.)

I'm debating with myself whether to replace by an asm() there. (If we expected
further uses of those overflow built-ins, we could consider adding non-built-
in fallbacks in those older branches. Yet unless something like this was needed
in an XSA, it would be solely 4.20 to gain such.)

Luckily in this case I think I would notice myself, as by default I'm building
the older trees with gcc 4.8.5 and 7.4.

Jan