Re: [PATCH v5] x86/nSVM: Check injected event consistency

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2026 11:38, Abdelkareem Abdelsaamad wrote:
> On the AMD platforms, allowing a VMRUN instruction with a malformed VMCB has
> debugging complications, security and performance implications. The APM
> volume #2 15.20 (40332—Rev. 4.10—July 2026) states the two possibilities that

Here and elsewhere: While doc rev plus section number are sufficient to
identify the section that is meant, that's still not very practical if
the reader only has some other revision to hand. Far in the past we did
establish that the main thing to mention is the section title. Those
tend to change very infrequently, if at all.

> VMRUN will immediately exit with VMEXIT_INVALID due to the injected event.
> These are
> • Reserved values of TYPE have been specified.
> • TYPE = 3 (exception) has been specified with a vector that does not
>   correspond to an exception (this includes vector 2, which is an NMI, not
>   an exception).
> 
> Furthermore, reading through the APM shows that the exception vector validity
> can also depend on the guest mode, the CPU generation and the
> microarchitectural constraints. Some vectors are only valid starting with the
> specific CPU generation that introduced the corresponding feature, the guest
> mode or whether the guest opted-in for specific CPU capability. The Upstream
> KVM introduced a similar consistency check that reflects on this dependency
> with the commit
> 7e79f71bca5c ("KVM: nSVM: Add missing consistency check for EVENTINJ").
> However, the KVM implementation did allow the Overflow (X86_EXC_OF) and the
> BOUND Range (X86_EXC_BR) vectors injection in 64-bit mode. According to the 
> APM
> Volume #2 and Volume #3 (40332—Rev. 4.10—July 2026), injecting these 
> exceptions
> while the guest is in 64-bit mode is invalid and triggers an immediate
> VMEXIT_INVALID. This architectural mismatch in the KVM implementation was
> reported here:
> https://lore.kernel.org/all/20260803225402.2324595-1-abdelkareem.abdelsaamad@xxxxxxxxxx/
> 
> To verify the hardware behaviour with the various vectors, A case-by-case
> testing on 64-bit mode Windows guest is performed. The hardware exception 
> event
> is manually injected at the end of svm_vmexit_handler and the consequence
> is observed:
>  - If VMEXIT_INVALID is triggered, the injection is invalid.
>  - If a guest triple fault is triggered, the event passed the hardware checks
>    and completed the event delivery. It is valid.
> 
> Extend the VMCB validation to check for the VMCB event injection 
> inconsistency.
> 
> While at it, extend the svm_vmcb_isvalid usage to the non-nested 
> VMEXIT_INVALID
> debugging. Clean up svm_vmcb_isvalid by dropping the unused `verbose` 
> parameter
> and by correcting the misleading boolean return value to properly reflect the
> validation status.

The dropping of that parameter wants splitting out. That's likely
uncontroversial. The new use of the function also would better be split
out (without folding with the parameter removal). And the polarity
inversion of the function's return value wants to be a separate patch,
too (arguably making the dropping of the parameter a secondary change
there might be okay).

> ---
> Testing:
>  - Using a locally developed XTF nested virt setup, I manually tested VMRUN
>    instruction handling with a malformed VMCB:
>    1) Inject event with the type (7).
>       The hypervisor logs show the message
>       (XEN) [  645.155609] d2v0[nsvm_vmcb_prepare4vmrun]: eventinj: Invalid
>             Injected Event Type: 0x7.
>    2) Inject event with the exception value (3) and the vector value (2) for
>       NMI. The hypervisor logs show the message
>       (XEN) [  645.157277] d2v0[nsvm_vmcb_prepare4vmrun]: eventinj: Invalid
>             exception type: 0x3 vector: 0x2.
>    3) Inject event with the exception value (3) and the vector value (21) for
>       the X86_EXC_CP (Control-Flow Protection).
>       Without the changes included:
>           On the Naples host, where the vector was not yet known to the
>           hardware. VMRUN immediately triggers VMEXIT_INVALID.
>    4) To perform more detailed bare-metal testing, I set up a testing matrix
>       using a XenServer Windows 10 64-bit VM and manually injected the
>       various events according to the testing matrix below:
> -----------------------------------------------------------------------------------
> |Exception Vector|     AMD Naples    |     AMD Genoa    |    Testing 
> Conditions    |
> |----------------|-------------------|------------------|--------------------------|
> |  - X86_EXC_OF  |  VMEXIT_INVALID   |  VMEXIT_INVALID  | - Verified that 
> 64bit    |
> |  - X86_EXC_BR  |                   |                  | mode is active 
> before    |
> |                |                   |                  | injecting the 
> event.     |
> |----------------|-------------------|------------------|--------------------------|
> |                |                   |                  | - Verified that the 
>      |
> |                |                   |                  | Genoa host does not 
> have |
> |  - X86_EXC_CP  | Guest Triple fault|  VMEXIT_INVALID  | guest_cr[4] 
> X86_CR4_CET  |
> |                |                   |                  | set and the VMCB 
> does not|
> |                |                   |                  | have CR4 
> X86_CR4_CET set |
> |----------------|-------------------|------------------|--------------------------|
> |                |                   |                  | - APM states only 
> valid  |
> | - X86_EXC_HV   |  VMEXIT_INVALID   |  VMEXIT_INVALID  | to inject into 
> VMSAs that|
> |                |                   |                  | execute with 
> Restricted  |
> |                |                   |                  | Injection.          
>      |
> |----------------|-------------------|------------------|--------------------------|
> | - X86_EXC_DE   |                   |                  | - Verified that 
> hosts    |
> | - X86_EXC_DB   |                   |                  | do not have 
> guest_cr[4]  |
> | - X86_EXC_UD   |                   |                  | X86_CR4_OSXMMEXCPT 
> set   |
> | - X86_EXC_BP   |                   |                  | and the VMCB does 
> not    |
> | - X86_EXC_NM   |                   |                  | have CR4            
>      |
> | - X86_EXC_DF   |                   |                  | X86_CR4_OSXMMEXCPT 
> set   |
> | - X86_EXC_TS   | Guest Triple fault|Guest Triple fault| with the vector     
>      |
> | - X86_EXC_NP   |                   |                  | X86_EXC_XM.         
>      |
> | - X86_EXC_SX   |                   |                  | with the vector     
>      |
> |                |                   |                  | X86_EXC_MC.         
>      |
> |----------------|-------------------|------------------|--------------------------|
> | - X86_EXC_CSO  |                   |                  |                     
>      |
> | - X86_EXC_SPV  |                   |                  |                     
>      |
> | - X86_EXC_VE   |  VMEXIT_INVALID   |  VMEXIT_INVALID  |                     
>      |
> | - X86_EXC_VC   |                   |                  |                     
>      |
>  
> ----------------------------------------------------------------------------------

Hmm, #XM and #MC don't appear in the leftmost column. They're mentioned in the
rightmost one, albeit there looks to be some corruption there.

Also, nit: The leading dashes in the leftmost column don't add any value, and
may hence better be omitted. Uniform leading padding there would further avoid
the (visual) impression that there's something wrong with the contents.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -327,20 +327,91 @@ void svm_vmcb_dump(const char *from, const struct 
> vmcb_struct *vmcb)
>      svm_dump_sel("  TR", &vmcb->tr);
>  }
>  
> +static bool is_valid_injected_exception_vector(const struct vmcb_struct 
> *vmcb,
> +    uint8_t vmcb_injected_vector, const struct vcpu *v)

If you indent the 2nd line like this, the first argument wants to also move
off of the 1st line:

static bool is_valid_injected_exception_vector(
    const struct vmcb_struct *vmcb,
    uint8_t vmcb_injected_vector,
    const struct vcpu *v)

Additionally, as shown, it is then also better to have one line per parameter
(albeit we may not be consistent there).

Alternatively:

static bool is_valid_injected_exception_vector(const struct vmcb_struct *vmcb,
                                               uint8_t vmcb_injected_vector,
                                               const struct vcpu *v)

> +{
> +    switch ( vmcb_injected_vector )
> +    {
> +    case X86_EXC_DE:
> +    case X86_EXC_DB:
> +    case X86_EXC_UD:
> +    case X86_EXC_NM:
> +    case X86_EXC_DF:
> +    case X86_EXC_TS:
> +    case X86_EXC_NP:
> +    case X86_EXC_SS:
> +    case X86_EXC_GP:
> +    case X86_EXC_PF:
> +    case X86_EXC_MF:
> +    case X86_EXC_AC:
> +    case X86_EXC_MC:
> +    case X86_EXC_XM:
> +    case X86_EXC_SX:
> +        return true;
> +
> +    /*
> +     * Exception vectors 3 and 4 may not be injected into SEV-ES guests.
> +     * If this is attempted, the VMRUN will fail with a VMEXIT_INVALID
> +     * error code.
> +     * See the APM Volume #2 15.35.8 (40332—Rev. 4.10—July 2026).
> +     */
> +    case X86_EXC_BP:
> +        return !vmcb_get_sev_es(vmcb);
> +
> +    /*
> +     * If the VMM attempts to inject an event that is impossible for the
> +     * guest mode (e.g., a #BR exception when the guest is in 64-bit mode),
> +     * the event injection will fail... VMRUN will immediately exit with
> +     * VMEXIT_INVALID.
> +     * See the APM Volume #2 15.20 (40332—Rev. 4.10—July 2026).
> +     */
> +    case X86_EXC_OF:
> +        return ( (!(vmcb_get_efer(vmcb) & EFER_LMA) || !vmcb->cs.l) &&
> +                 !vmcb_get_sev_es(vmcb) );

Nit: Blanks immediately inside parentheses are to be used only in for(),
if(), etc.

> +    case X86_EXC_BR:
> +        return !(vmcb_get_efer(vmcb) & EFER_LMA) || !vmcb->cs.l;

I'd like to suggest to combine #OF and #BR handling (to also better fit
commentary), e.g.:

    case X86_EXC_OF:
        if ( vmcb_get_sev_es(vmcb) )
            return false;
        fallthrough;
    case X86_EXC_BR:
        return !(vmcb_get_efer(vmcb) & EFER_LMA) || !vmcb->cs.l;

> +    case X86_EXC_CP:
> +    {
> +        unsigned long guest_valid_cr4 = hvm_cr4_guest_valid_bits(v->domain);
> +        return guest_valid_cr4 & X86_CR4_CET;
> +    }

I continue to disagree. While this tries to match what the table says, it
still isn't correct to key this to the bits a guest _might_ set. Imo AMD
have screwed up there, and #CP shouldn't have been valid to inject on
pre-CET hardware. For our purposes, consider a guest migrating from a
Naples host to a Genoa one: It'll suddenly observe different behavior (if
#CP injection was attempted in the first place).

This will then also fit the returning of "false" in the default case.
Prior to the introduction of #CP and CR4.CET the handling of vector 0x15
would also have ended up there, after all. Such a hypothetical "old" Xen
would have behaved uniformly on CET-incapable and CET-capable hardware.
(The same obviously is true for any new exception types we may get to see
down the road.)

Also, nit: Blank line please between declaration(s) and statement(s).

> +    /*
> +     * X86_EXC_HV vector is reserved for SNP guests use. Only allowed to be
> +     * injected into VMSAs that execute with Restricted Injection.
> +     * See the APM Volume #2 15.36 (40332—Rev. 4.10—July 2026).
> +     */
> +    case X86_EXC_HV:
> +    default:
> +        return false;
> +    }
> +}
> +
>  bool svm_vmcb_isvalid(
> -    const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v,
> -    bool verbose)
> +    const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v)
>  {
> -    bool ret = false; /* ok */
> +    bool ret = true; /* ok */
>      unsigned long cr0 = vmcb_get_cr0(vmcb);
>      unsigned long cr3 = vmcb_get_cr3(vmcb);
>      unsigned long cr4 = vmcb_get_cr4(vmcb);
>      unsigned long valid;
>      uint64_t efer = vmcb_get_efer(vmcb);
> +    unsigned int vmcb_injected_type = vmcb->event_inj.type;
> +    unsigned int vmcb_injected_vector = vmcb->event_inj.vector;
> +    /*
> +     * Represent the nonreserved and accoridngly valid guest exception or
> +     * interrupt type.
> +     * See the APM Volume #2 15.20 (40332—Rev. 4.10—July 2026).
> +     */
> +    unsigned long vmcb_valid_event_inj_types_mask = (1 << X86_ET_INTR) |
> +                                                    (1 << X86_ET_NMI) |
> +                                                    (1 << X86_ET_HW_EXC) |
> +                                                    (1 << X86_ET_SW_INT);

Why unsigned long when unsigned int would do? There's also no need for ...

> @@ -399,6 +470,15 @@ bool svm_vmcb_isvalid(
>          PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
>                 vmcb->event_inj.raw);
>  
> +    if ( !((1ul << vmcb_injected_type) & vmcb_valid_event_inj_types_mask) )

... a ul suffix here (which btw would want to be UL for Misra's sake, if a
prefix was necessary in the first place).

> +        PRINTF("eventinj: Invalid Injected Event Type: %#x\n",
> +               vmcb_injected_type);
> +
> +    if ( (vmcb_injected_type == X86_ET_HW_EXC) &&
> +         !is_valid_injected_exception_vector(vmcb, vmcb_injected_vector, v) )
> +        PRINTF("eventinj: Invalid exception type: %#x vector: %#x\n",
> +               vmcb_injected_type, vmcb_injected_vector);
Logging vmcb_injected_type here isn't very useful: It's known to be
X86_ET_HW_EXC.

Jan