Re: [PATCH v2 05/10] KVM: selftests: Reuse GPR switching logic for nVMX

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `guest_regs` array is sized for 15 elements, but 16 elements are accessed, causing an out-of-bounds read/write to the `.bss` section.
- [Medium] `vmlaunch()` and `vmresume()` behavior for guest GPRs diverges based on the `enable_evmcs` flag, silently failing tests that rely on the new GPR switching logic under Enlightened VMCS.
--

commit 496a5916183241f499c38e4f427dd5cbecd4dc37
Author: Yosry Ahmed <yosry@kernel.org>

    KVM: selftests: Reuse GPR switching logic for nVMX
    
    Reuse the GPR switching logic for nVMX by defining VMX_SWITCH_GPRS_ASM,
    which is essentially the same as SVM_SWITCH_GPRS_ASM but also switches
    RAX, replacing the push/pop of a subset of the registers.
    
    The long clobber list of registers is no longer needed as registers are
    saved and restored appropriately (and not clobbered by L2).

> diff --git a/tools/testing/selftests/kvm/include/x86/vmx.h b/tools/testing/selftests/kvm/include/x86/vmx.h
> index 90fffaf915958..64996c310fe4c 100644
> --- a/tools/testing/selftests/kvm/include/x86/vmx.h
> +++ b/tools/testing/selftests/kvm/include/x86/vmx.h
> @@ -363,9 +363,23 @@ static inline u64 vmptrstz(void)
>  	return value;
>  }
>  
> -/*
> - * No guest state (e.g. GPRs) is established by this vmlaunch.
> - */
> +#define VMX_SWITCH_GPRS_ASM \
> +	GUEST_SWITCH_GPR_ASM(rax, GUEST_REGS_RAX) \
> +	GUEST_SWITCH_GPR_ASM(rbx, GUEST_REGS_RBX) \
> +	GUEST_SWITCH_GPR_ASM(rcx, GUEST_REGS_RCX) \
> +	GUEST_SWITCH_GPR_ASM(rdx, GUEST_REGS_RDX) \
> +	GUEST_SWITCH_GPR_ASM(rbp, GUEST_REGS_RBP) \
> +	GUEST_SWITCH_GPR_ASM(rsi, GUEST_REGS_RSI) \
> +	GUEST_SWITCH_GPR_ASM(rdi, GUEST_REGS_RDI) \
> +	GUEST_SWITCH_GPR_ASM(r8,  GUEST_REGS_R8)  \
> +	GUEST_SWITCH_GPR_ASM(r9,  GUEST_REGS_R9)  \
> +	GUEST_SWITCH_GPR_ASM(r10, GUEST_REGS_R10) \
> +	GUEST_SWITCH_GPR_ASM(r11, GUEST_REGS_R11) \
> +	GUEST_SWITCH_GPR_ASM(r12, GUEST_REGS_R12) \
> +	GUEST_SWITCH_GPR_ASM(r13, GUEST_REGS_R13) \
> +	GUEST_SWITCH_GPR_ASM(r14, GUEST_REGS_R14) \
> +	GUEST_SWITCH_GPR_ASM(r15, GUEST_REGS_R15)

[Severity: High]
Does this access past the end of the guest_regs array?

GUEST_REGS_R15 evaluates to 15, and the array is defined in
tools/testing/selftests/kvm/include/x86/processor.h as:

#define NR_GUEST_REGS	GUEST_REGS_R15
extern u64 guest_regs[NR_GUEST_REGS];

Since NR_GUEST_REGS is 15, the array only holds 15 elements (indices 0 to 14).
Calling GUEST_SWITCH_GPR_ASM(r15, GUEST_REGS_R15) will read and write the 16th
element at index 15, resulting in an out-of-bounds access that corrupts memory
in the .bss section.

> +
>  static inline int vmlaunch(void)
>  {
>  	int ret;
> @@ -373,34 +387,23 @@ static inline int vmlaunch(void)
>  	if (enable_evmcs)
>  		return evmcs_vmlaunch();

[Severity: Medium]
Will this break EVMCS test configurations?

If enable_evmcs is true, the code redirects to evmcs_vmlaunch() which wasn't
updated to use VMX_SWITCH_GPRS_ASM.  It still uses the old inline assembly
that completely ignores guest_regs:

tools/testing/selftests/kvm/include/x86/evmcs.h:evmcs_vmlaunch() {
    ...
    __asm__ __volatile__("push %%rbp;"
                 "push %%rcx;"
                 "push %%rdx;"
                 "push %%rsi;"
                 "push %%rdi;"
                 "push $0;"
    ...
}

Any KVM selftest that relies on guest_regs to establish L2 state before VM
entry will now silently execute with uninitialized L2 GPRs when run with EVMCS
enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604203546.365658-1-yosry@kernel.org?part=5