[PATCH] KVM: selftests: Fix sign extension bug in get_desc64_base()

[PATCH] KVM: selftests: Fix sign extension bug in get_desc64_base()

[MJ Pooladkhay]

The function get_desc64_base() performs a series of bitwise left shifts on
fields of various sizes. More specifically, when performing '<< 24' on
'desc->base2' (which is a u8), 'base2' is promoted to a signed integer
before shifting.

In a scenario where base2 >= 0x80, the shift places a 1 into bit 31,
causing the 32-bit intermediate value to become negative. When this
result is cast to uint64_t or ORed into the return value, sign extension
occurs, corrupting the upper 32 bits of the address (base3).

Example:
Given:
  base0 = 0x5000
  base1 = 0xd6
  base2 = 0xf8
  base3 = 0xfffffe7c

Expected return: 0xfffffe7cf8d65000
Actual return:   0xfffffffff8d65000

Fix this by explicitly casting the fields to 'uint64_t' before shifting
to prevent sign extension.

Signed-off-by: MJ Pooladkhay <mj@pooladkhay.com>
---
While using get_desc64_base() to set the HOST_TR_BASE value for a custom 
educational hypervisor, I observed system freezes, either immediately or
after migrating the guest to a new core. I eventually realized that KVM
uses get_cpu_entry_area() for the TR base. Switching to that fixed my
freezes (which were triple faults on one core followed by soft lockups 
on others, waiting on smp_call_function_many_cond) and helped me identify
the sign-extension bug in this helper function that was corrupting the
HOST_TR_BASE value.

Thanks,
MJ Pooladkhay

 tools/testing/selftests/kvm/include/x86/processor.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
index 57d62a425..cc2f8fb6f 100644
--- a/tools/testing/selftests/kvm/include/x86/processor.h
+++ b/tools/testing/selftests/kvm/include/x86/processor.h
@@ -436,8 +436,11 @@ struct kvm_x86_state {
 
 static inline uint64_t get_desc64_base(const struct desc64 *desc)
 {
-	return ((uint64_t)desc->base3 << 32) |
-		(desc->base0 | ((desc->base1) << 16) | ((desc->base2) << 24));
+	uint64_t low = (uint64_t)desc->base0 |
+		       ((uint64_t)desc->base1 << 16) |
+		       ((uint64_t)desc->base2 << 24);
+
+	return (uint64_t)desc->base3 << 32 | low;
 }
 
 static inline uint64_t rdtsc(void)
-- 
2.52.0

Re: [PATCH] KVM: selftests: Fix sign extension bug in get_desc64_base()

[Sean Christopherson]

On Sat, Dec 20, 2025, MJ Pooladkhay wrote:
> The function get_desc64_base() performs a series of bitwise left shifts on
> fields of various sizes. More specifically, when performing '<< 24' on
> 'desc->base2' (which is a u8), 'base2' is promoted to a signed integer

Ugh, I hate integer promotion rules.  I wish there was a more useful version of
-Wconversion :-/

> before shifting.
> 
> In a scenario where base2 >= 0x80, the shift places a 1 into bit 31,
> causing the 32-bit intermediate value to become negative. When this
> result is cast to uint64_t or ORed into the return value, sign extension
> occurs, corrupting the upper 32 bits of the address (base3).
> 
> Example:
> Given:
>   base0 = 0x5000
>   base1 = 0xd6
>   base2 = 0xf8
>   base3 = 0xfffffe7c
> 
> Expected return: 0xfffffe7cf8d65000
> Actual return:   0xfffffffff8d65000
> 
> Fix this by explicitly casting the fields to 'uint64_t' before shifting
> to prevent sign extension.
> 
> Signed-off-by: MJ Pooladkhay <mj@pooladkhay.com>
> ---
> While using get_desc64_base() to set the HOST_TR_BASE value for a custom 
> educational hypervisor, I observed system freezes, either immediately or
> after migrating the guest to a new core. I eventually realized that KVM
> uses get_cpu_entry_area() for the TR base. Switching to that fixed my
> freezes (which were triple faults on one core followed by soft lockups 
> on others, waiting on smp_call_function_many_cond) and helped me identify
> the sign-extension bug in this helper function that was corrupting the
> HOST_TR_BASE value.
> 
> Thanks,
> MJ Pooladkhay
> 
>  tools/testing/selftests/kvm/include/x86/processor.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
> index 57d62a425..cc2f8fb6f 100644
> --- a/tools/testing/selftests/kvm/include/x86/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86/processor.h
> @@ -436,8 +436,11 @@ struct kvm_x86_state {
>  
>  static inline uint64_t get_desc64_base(const struct desc64 *desc)
>  {
> -	return ((uint64_t)desc->base3 << 32) |
> -		(desc->base0 | ((desc->base1) << 16) | ((desc->base2) << 24));
> +	uint64_t low = (uint64_t)desc->base0 |
> +		       ((uint64_t)desc->base1 << 16) |
> +		       ((uint64_t)desc->base2 << 24);
> +
> +	return (uint64_t)desc->base3 << 32 | low;

I don't see any reason to have an intermediate "low", it just makes it harder
to piece the entire thing together.  My vote is for:

	return (uint64_t)desc->base3 << 32 |
	       (uint64_t)desc->base2 << 24 |
	       (uint64_t)desc->base1 << 16 |
	       (uint64_t)desc->base0;

>  }
>  
>  static inline uint64_t rdtsc(void)
> -- 
> 2.52.0
> 

Re: [PATCH] KVM: selftests: Fix sign extension bug in get_desc64_base()

[MJ Pooladkhay]

On Mon, Dec 22, 2025, Sean Christopherson wrote:

> Ugh, I hate integer promotion rules.  I wish there was a more useful version of
> -Wconversion :-/
True! It caused me weeks of grief tracking down these triple faults!
> I don't see any reason to have an intermediate "low", it just makes it harder
> to piece the entire thing together.  My vote is for:
>
> 	return (uint64_t)desc->base3 << 32 |
> 	       (uint64_t)desc->base2 << 24 |
> 	       (uint64_t)desc->base1 << 16 |
> 	       (uint64_t)desc->base0;

Thanks for the review. I agree, your version is much cleaner. I will 
send a v2 with this change shortly.

Best,
MJ Pooladkhay