Re: [PATCH v1] nSVM: Check injected event consistency

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 15/07/2026 à 15:39, Abdelkareem Abdelsaamad a écrit :
> 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 [1] states two possibilities that result in a VMRUN exit with
> VMEXIT_INVALID due to injected events. These are either
> • 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 checks to check for such inconsistency.
>
> The collection of the invalid exception vectors are picked from the upstream KVM
> commit ("7e79f71bca5c" KVM: nSVM: Add missing consistency check for EVENTINJ).
>
> [1] https://docs.amd.com/v/u/en-US/24593_3.44_APM_Vol2
>
> Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>
> ---
> 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 
> Injected Event.
>               Exception type: (0x3), with a vector: (0x2) does not belong to an 
> exception
>
>   - CI tests:
> https://gitlab.com/xen-project/people/aabdelsa/xen/-/pipelines/2678629270
> ---
>   xen/arch/x86/hvm/svm/vmcb.c | 36 ++++++++++++++++++++++++++++++++++++
>   xen/arch/x86/hvm/svm/vmcb.h |  3 +++
>   2 files changed, 39 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 975a1eaef8..1ca8474958 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -320,6 +320,31 @@ 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(
> +    const struct vmcb_struct *vmcb, uint8_t vmcb_injected_vector)
> +{
> +    return ( (vmcb_injected_vector == X86_EXC_DE) ||
> +             (vmcb_injected_vector == X86_EXC_DB) ||
> +             (vmcb_injected_vector == X86_EXC_BP) ||
> +             (vmcb_injected_vector == X86_EXC_OF) ||
> +             (vmcb_injected_vector == X86_EXC_BR) ||
> +             (vmcb_injected_vector == X86_EXC_UD) ||
> +             (vmcb_injected_vector == X86_EXC_NM) ||
> +             (vmcb_injected_vector == X86_EXC_DF) ||
> +             (vmcb_injected_vector == X86_EXC_TS) ||
> +             (vmcb_injected_vector == X86_EXC_NP) ||
> +             (vmcb_injected_vector == X86_EXC_SS) ||
> +             (vmcb_injected_vector == X86_EXC_GP) ||
> +             (vmcb_injected_vector == X86_EXC_PF) ||
> +             (vmcb_injected_vector == X86_EXC_MF) ||
> +             (vmcb_injected_vector == X86_EXC_AC) ||
> +             (vmcb_injected_vector == X86_EXC_MC) ||
> +             (vmcb_injected_vector == X86_EXC_XM) ||
> +             (vmcb_injected_vector == X86_EXC_HV) ||
> +             (vmcb_injected_vector == X86_EXC_SX) ||
> +             (vmcb_get_sev_es(vmcb) && vmcb_injected_vector == X86_EXC_VC) );
I'm not sure that #VC is gated behind SEV-ES guest.

Regardless, injecting manually #VC is weird (especially for SEV-ES 
guests where it's strongly discouraged), but it could be somewhat abused 
to emulate some of the SEV-ES semantics. So I would suggest to threat it 
like the others.
> +}
> +
>   bool svm_vmcb_isvalid(
>       const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v,
>       bool verbose)
> @@ -330,6 +355,8 @@ bool svm_vmcb_isvalid(
>       unsigned long cr4 = vmcb_get_cr4(vmcb);
>       unsigned long valid;
>       uint64_t efer = vmcb_get_efer(vmcb);
> +    uint8_t vmcb_injected_type = vmcb->event_inj.type & 
> SVM_EVENT_INJ_TYPE_MASK;
> +    uint8_t vmcb_injected_vector = vmcb->event_inj.vector & 
> SVM_EVENT_INJ_VEC_MASK;
What are these masks for ?

The definition of vmcb->event_inj suggest that they are not required
> uint8_t  vector;
> uint8_t  type:3;

>   
>   #define PRINTF(fmt, args...) do { \
>       if ( !verbose ) return true; \
> @@ -392,6 +419,15 @@ bool svm_vmcb_isvalid(
>           PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
>                  vmcb->event_inj.raw);
>   
> +    if ( vmcb_injected_type > X86_ET_SW_INT )
> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
> +               vmcb_injected_type);
What about vmcb_injected_type == 1 case ?

> +
> +    if ( (vmcb_injected_type == X86_ET_HW_EXC) &&
> +         !is_valid_svm_vmcb_injected_exception_vector(vmcb, 
> vmcb_injected_vector) )
> +        PRINTF("eventinj: Invalid Injected Event. Exception type: (%#"PRIx8"),"
> +               " with a vector: (%#"PRIx8") does not belong to an exception\n",
> +               vmcb_injected_type, vmcb_injected_vector);
>   #undef PRINTF
>       return ret;
>   }
> diff --git a/xen/arch/x86/hvm/svm/vmcb.h b/xen/arch/x86/hvm/svm/vmcb.h
> index 13ccfd3ff9..3c22095db1 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/hvm/svm/vmcb.h
> @@ -306,6 +306,9 @@ enum
>       TLB_CTRL_FLUSH_ASID_NONGLOBAL = 7,
>   };
>   
> +#define SVM_EVENT_INJ_VEC_MASK     0xff
> +#define SVM_EVENT_INJ_TYPE_MASK    0x7
> +
>   typedef union
>   {
>       struct
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature