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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.08.2026 19:23, 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
> result in a VMRUN 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).
> 
> Extend the VMCB validation to check for such inconsistency.
> 
> The collection of the invalid exception vectors are ported from the upstream
> KVM commit
> ("7e79f71bca5c" KVM: nSVM: Add missing consistency check for EVENTINJ). Adjust
> the checks from the commit to align with the APM Volume #2 and Volume #3
> (40332—Rev. 4.40—July 2026) for the X86_EXC_OF and X86_EXC_BR vectors which

Isn't this 4.10, just like you have it further up?

> should not be valid on the x86 64-bit (long mode) platforms. The adjustment is
> posted to the KVM mailing commit patch thread
> https://lore.kernel.org/all/20260803225402.2324595-1-abdelkareem.abdelsaamad@xxxxxxxxxx/
> 
> Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>
> ---
> Changes in v3:
> - Restricted X86_EXC_OF (4) and X86_EXC_BR (5) vector injections to 
>   non-64-bit guests to prevent impossible guest-mode state injections
>   per AMD APM Volumes 2 & 3.
> - Refactored exception vector validation from if-conditions to a switch
>   statement to improve readability and extensibility.
> - Restricted X86_EXC_CP (21) vector injection to hosts with enabled CET
>   to prevent VMRUN failures on hardware without CET support.

When reading this, I was puzzled, but the code below is correct: It's not
the host you check, but the guest's CR4.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -320,6 +320,41 @@ void svm_vmcb_dump(const char *from, const struct 
> vmcb_struct *vmcb)
>      svm_dump_sel("  TR", &vmcb->tr);
>  }
>  
> +static bool is_valid_svm_vmcb_injected_exception_vector(

Is in particular "svm" but perhaps also "vmcb" really relevant in the name
here (which is a static helper)?

> +    const struct vmcb_struct *vmcb, uint8_t vmcb_injected_vector)
> +{
> +    switch ( vmcb_injected_vector )
> +    {
> +    case X86_EXC_DE:
> +    case X86_EXC_DB:
> +    case X86_EXC_BP:
> +    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:

Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?

> +    case X86_EXC_HV:
> +    case X86_EXC_SX:

Are #HV and #SX really permitted without any constraints?

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

Nit: No need for parentheses on the rhs of the ||.

> +    case X86_EXC_VC:
> +        return vmcb_get_sev_es(vmcb);
> +    case X86_EXC_CP:
> +        return !!(vmcb_get_cr4(vmcb) & X86_CR4_CET);

No need for !! when converting to bool.

> +    default:
> +        return false;
> +    }

Throughout: Blank lines please between non-fall-through case blocks.

> @@ -392,6 +433,16 @@ bool svm_vmcb_isvalid(
>          PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
>                 vmcb->event_inj.raw);
>  
> +    if ( !((1 << vmcb_injected_type) & vmcb_valid_event_inj_types_mask) )
> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
> +               vmcb_injected_type);
> +
> +    if ( (vmcb_injected_type == X86_ET_HW_EXC) &&
> +         !is_valid_svm_vmcb_injected_exception_vector(
> +             vmcb, vmcb_injected_vector) )

Nit: Indentation is off by one here. The anchor on the earlier line isn't the
'!' but the 'i'.

> +        PRINTF("eventinj: Invalid Injected Event. Exception type: 
> (%#"PRIx8"),"
> +               " with a vector: (%#"PRIx8") does not belong to an exception 
> on"
> +               " the platform \n", vmcb_injected_type, vmcb_injected_vector);

This message is quite a bit too long. There's also a stray blank ahead of the
\n. And further arguments after one that was already wrapped across lines want
to start on a separate line.

Jan