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 <[email protected]> > --- > 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 >
