Re: [PATCH] x86/vmx: Fix cascade crash in vmx_vmentry_failure()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2026 21:35, Andrew Cooper wrote:
> The VMEntry failure handling does not distinguish VMFail Valid vs Invalid.  In
> the latter case, vmx_vmentry_failure() will hit a BUG() when trying to look up
> VM_INSTRUCTION_ERROR.

That's the BUG() in __vmread(), I expect? I suppose this is also connected to 
...

> Slightly RFC - it's fairly hard to these these.  It came from code inspection
> rather than encountering a VMFailInvalid case in practice.

... this. (Presumably the first "these" also was meant to be "observe" or "hit"
or some such.) What I'm having difficulty with is seeing how we would make it
as far: If said __vmread() hit its BUG(), wouldn't all earlier VMCS accesses
(reads or writes) have hit that, too? I.e. would this be guarding us merely
against a pretty narrow window where memory corruption might occur? And
wouldn't that then be as little or as much of a reason for crashing as any
other failed VM{READ,WRITE}?

> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -154,21 +154,7 @@ UNLIKELY_END(realmode)
>  
>  .Lvmx_launch:
>          vmlaunch
> -
> -.Lvmx_vmentry_fail:
> -        sti
> -        PUSH_AND_CLEAR_GPRS
> -
> -        /*
> -         * SPEC_CTRL_ENTRY notes
> -         *
> -         * If we end up here, no guest code has executed.  The MSR lists have
> -         * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
> -         * in context, and the RSB is unchanged.
> -         */
> -
> -        call vmx_vmentry_failure
> -        jmp  .Lvmx_process_softirqs
> +        jmp  .Lvmx_vmentry_fail
>  
>  LABEL(vmx_asm_do_vmentry)
>          GET_CURRENT(bx)
> @@ -189,3 +175,30 @@ LABEL(vmx_asm_do_vmentry)
>          call do_softirq
>          jmp  .Lvmx_do_vmentry
>  END(vmx_asm_vmexit_handler)
> +
> +        .section .text.cold, "ax", @progbits
> +
> +FUNC(vmx_asm_vmexit_handler.cold)

Is this doing what you want when CC_SPLIT_SECTIONS=y (and assuming my patch
to extend this to assembly code would finally land)?

> +.Lvmx_vmentry_fail:
> +        /*
> +         * SPEC_CTRL_ENTRY notes
> +         *
> +         * If we end up here, no guest code has executed.  The MSR lists have
> +         * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
> +         * in context, and the RSB is unchanged.

How do we know the MSR lists haven't been processed? VM entry can fail because
of that processing. Afaict we only know this in the "VMfail invalid" case. For
SPEC_CTRL (in the "VMfail valid" case) this means we only have Xen's choice
still in context if that's the last entry on the list.

> +         * The guest registers are live, and the on-stack copy is up-to-date.
> +         * PUSH_AND_CLEAR_GPRS clobbers flags and can't reasonably be made 
> not
> +         * to.  The Carry flag (VMFail Invalid vs Valid) needs preserving.

Define "reasonably". What about (with my CET-SS part included)

.macro PUSH_AND_CLEAR_GPRS ssp=IS_ENABLED(CONFIG_XEN_SHSTK)
        push  %rdi
        mov   $0, %edi
        push  %rsi
        mov   %edi, %esi
        push  %rdx
        mov   %edi, %edx
        push  %rcx
        mov   %edi, %ecx
        push  %rax
        mov   %edi, %eax
 .if \ssp
        rdsspq %rcx
 .endif
        push  %r8
        mov   %edi, %r8d
        push  %r9
        mov   %edi, %r9d
        push  %r10
        mov   %edi, %r10d
        push  %r11
        mov   %edi, %r11d
        push  %rbx
        mov   %edi, %ebx
        push  %rbp
#ifdef CONFIG_FRAME_POINTER
/* Indicate special exception stack frame by inverting the frame pointer. */
        mov   %rsp, %rbp
        not   %rbp
#else
        mov   %edi, %ebp
#endif
        push  %r12
        mov   %edi, %r12d
        push  %r13
        mov   %edi, %r13d
        push  %r14
        mov   %edi, %r14d
        push  %r15
        mov   %edi, %r15d
#ifdef CONFIG_XEN_SHSTK
        push  %rcx
#endif
.endm

? Whether we'd accept the slightly longer form everywhere, or whether we'd
prefer to add a parameter is TBD.

> +         * We could opencode PUSH_AND_CLEAR_GPRS but that's fragile to stack
> +         * layout changes.  Instead, use a spare byte in the cpuinfo block.
> +         */
> +        setnc   STK_REL(CPUINFO_vmx_vmfail_valid, CPUINFO_error_code)(%rsp)
> +
> +        PUSH_AND_CLEAR_GPRS
> +        sti

Any reason you have re-ordered these two? Can't STI still be the very first
insn after the label? (Of course, if - as per above - SPEC_CTRL would first
need restoring, that would likely need to be ahead of STI.)

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1833,18 +1833,53 @@ void vmx_destroy_vmcs(struct vcpu *v)
>      free_xenheap_page(v->arch.hvm.vmx.msr_bitmap);
>  }
>  
> -void vmx_vmentry_failure(void)
> +static const char *vmx_error_str(unsigned int error)
> +{
> +    switch ( error )
> +    {
> +    case VMX_INSN_VMLAUNCH_NONCLEAR_VMCS:
> +        return "VMLAUNCH with non-clear VMCS";
> +
> +    case VMX_INSN_VMRESUME_NONLAUNCHED_VMCS:
> +        return "VMRESUME with non-launched VMCS";
> +
> +    case VMX_INSN_VMRESUME_AFTER_VMXOFF:
> +        return "VMRESUME after VMXOFF";
> +
> +    case VMX_INSN_INVALID_CONTROL_STATE:
> +        return "Invalid control state";
> +
> +    case VMX_INSN_INVALID_HOST_STATE:
> +        return "Invalid host state";
> +
> +    case VMX_INSN_VMENTRY_BLOCKED_BY_MOV_SS:
> +        return "Blocked by MOV-SS";
> +
> +    default:
> +        return "Unknown";
> +    }
> +}
> +
> +void asmlinkage __cold vmx_vmentry_failure(void)
>  {
>      struct vcpu *curr = current;
> -    unsigned long error;
> +    bool valid = get_cpu_info()->vmx_vmfail_valid;
>  
> -    __vmread(VM_INSTRUCTION_ERROR, &error);
> -    gprintk(XENLOG_ERR, "VM%s error: %#lx\n",
> -            curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH", error);
> +    gprintk(XENLOG_ERR, "VM%s Failure, VMCS %svalid\n",
> +            curr->arch.hvm.vmx.launched ? "RESUME" : "LAUNCH",
> +            valid ? "" : "not ");
>  
> -    if ( error == VMX_INSN_INVALID_CONTROL_STATE ||
> -         error == VMX_INSN_INVALID_HOST_STATE )
> -        vmcs_dump_vcpu(curr);
> +    if ( valid )
> +    {
> +        unsigned int error = vmread(VM_INSTRUCTION_ERROR);
> +
> +        gprintk(XENLOG_ERR, "  Instruction Error %u, %s\n",
> +                error, vmx_error_str(error));

With this being the only call to vmx_error_str(), would that better also be
__cold (despite the compiler almost certainly inlining it, unless __cold
triggered some special inlining decisions)?

> --- a/xen/arch/x86/include/asm/current.h
> +++ b/xen/arch/x86/include/asm/current.h
> @@ -80,6 +80,9 @@ struct cpu_info {
>       */
>      bool         use_pv_cr3;
>  
> +    /* Scratch space for the VT-x logic.  See users. */
> +    uint8_t      vmx_vmfail_valid;

Any reason this isn't bool? No use of the field wants it differently afaics.

Should we - mostly for doc purposes - also wrap this in #ifdef CONFIG_VMX?

Jan