linux-kernel - Re: [PATCH] KVM: selftests: Fix sign extension bug in get_desc64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aUlt4zOuyQ2WNiRe@google.com>
Date: Mon, 22 Dec 2025 08:12:19 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: MJ Pooladkhay <mj@...ladkhay.com>
Cc: pbonzini@...hat.com, shuah@...nel.org, kvm@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: selftests: Fix sign extension bug in get_desc64_base()

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@...ladkhay.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
>