Re: [PATCH 4/4] KVM: selftests: Check VMPTRLD with active eVMCS

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The hardcoded `VMPTRLD_INSN_SIZE` of 5 bytes is brittle because `vmptrld` is executed using an inline assembly memory operand constraint (`"m"`), which results in a variable-sized instruction depending on compiler optimizations and register allocation.
--

--- Patch [4]: [PATCH 4/4] KVM: selftests: Check VMPTRLD with active eVMCS ---
commit 80c84d1f71bebfd26712329b4fe8a487216ec3fb
Author: Vitaly Kuznetsov <[email protected]>

KVM: selftests: Check VMPTRLD with active eVMCS

Check that VMPTRLD when eVMCS is active results in #UD. This matches
genuine Hyper-V's behavior.

> diff --git a/tools/testing/selftests/kvm/x86/hyperv_evmcs.c b/tools/testing/selftests/kvm/x86/hyperv_evmcs.c
> index 88262ddf7fcb2..6a6d940a78918 100644
> --- a/tools/testing/selftests/kvm/x86/hyperv_evmcs.c
> +++ b/tools/testing/selftests/kvm/x86/hyperv_evmcs.c
> @@ -18,12 +18,16 @@
>  #include "hyperv.h"
>  #include "vmx.h"
>  
> +#define VMLAUNCH_INSN_SIZE 3
> +#define VMPTRLD_INSN_SIZE 5
> +
>  static int ud_count;
> +static int ud_insn_skip_size;
>  
>  static void guest_ud_handler(struct ex_regs *regs)
>  {
>  	ud_count++;
> -	regs->rip += 3; /* VMLAUNCH */
> +	regs->rip += ud_insn_skip_size;
>  }

[ ... ]

> @@ -182,10 +186,15 @@ void guest_code(struct vmx_pages *vmx_pages, struct hyperv_test_pages *hv_pages,
>  	GUEST_ASSERT(vmreadz(VM_EXIT_REASON) == EXIT_REASON_VMCALL);
>  	GUEST_SYNC(11);
>  
> +	ud_insn_skip_size = VMPTRLD_INSN_SIZE;
> +	vmptrld(hv_pages->enlightened_vmcs_gpa);

[Severity: Medium]
Could this hardcoded instruction size lead to guest instruction stream
corruption?

The vmptrld() inline assembly in include/x86/vmx.h uses a memory operand
constraint ("m") for the physical address:

static inline int vmptrld(u64 vmcs_pa)
{
	u8 ret;

	__asm__ __volatile__ ("vmptrld %[pa]; setna %[ret]"
		: [ret]"=rm"(ret)
		: [pa]"m"(vmcs_pa)
		: "cc", "memory");
        ...
}

Depending on compiler optimizations and register allocation, the resulting
instruction might not be exactly 5 bytes in size (for example, it could be 4
or 7 bytes depending on the chosen addressing mode).

Since guest_ud_handler() unconditionally advances regs->rip by
ud_insn_skip_size, if the generated instruction length does not match
VMPTRLD_INSN_SIZE, resuming the guest could land in the middle of an
instruction and cause #GP faults or infinite #UD loops.

> +	GUEST_ASSERT(ud_count == 1);
> +
>  	/* Try enlightened vmptrld with an incorrect GPA */
> +	ud_insn_skip_size = VMLAUNCH_INSN_SIZE;
>  	evmcs_vmptrld(0xdeadbeef, hv_pages->enlightened_vmcs);
>  	GUEST_ASSERT(vmlaunch());
> -	GUEST_ASSERT(ud_count == 1);
> +	GUEST_ASSERT(ud_count == 2);
>  	GUEST_DONE();
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4