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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2026 18:11, 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.10—July 2026) for the X86_EXC_OF and X86_EXC_BR vectors which
> 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/
> 
> Injecting the vector X86_EXC_HV is also found to trigger VMEXIT_INVALID with
> the Xen hypervisor. Drop the X86_EXC_HV vector from the permitted vectors.

Is this matched by anything in the PM? There is "#HV is only allowed to be
injected into VMSAs that execute with Restricted Injection." Which suggests
#HV can be injected, but only under a certain condition. Following what
Teddy said towards v3, this may want expressing by a separate case block
also returning false, but having a comment.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -320,6 +320,44 @@ 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)
> +{
> +    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:

Is #MC valid to inject without CR4.MCE set?

> +    case X86_EXC_XM:

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

> +    case X86_EXC_SX:

Again as before: Is #SX really permitted without any constraints? You did
reply to both comments on v3, but that outcome isn't reflected here. The
more that what you said there could equally apply ...

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

... e.g. here. That is, if a CR4 (or other) check is needed here, but not
for #XM (or #SX), that's surely worth (briefly) commenting upon. The more
that, afaics, none of this is spelled out in the PM.

> @@ -330,6 +368,12 @@ 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;
> +    uint8_t vmcb_injected_vector = vmcb->event_inj.vector;
> +    uint8_t vmcb_valid_event_inj_types_mask = (1 << X86_ET_EXT_INTR) |
> +                                              (1 << X86_ET_NMI) |
> +                                              (1 << X86_ET_HW_EXC) |
> +                                              (1 << X86_ET_SW_INT);

The absence of X86_ET{_PRIV,}_SW_EXC likely wants a brief comment, as that's
a peculiarity of SVM. Alternatively how about introducing X86_ET_SVM_ALL (or
some such) as a #define somewhere?

> @@ -392,6 +436,20 @@ bool svm_vmcb_isvalid(
>          PRINTF("eventinj: MBZ bits are set (%#"PRIx64")\n",
>                 vmcb->event_inj.raw);
>  
> +    if ( !vmcb->event_inj.v )
> +        PRINTF("eventinj: valid bit is not set (%#"PRIx64")\n",
> +               vmcb->event_inj.raw);

I understand the parentheses in the log message here. Yet ...

> +    if ( !((1 << vmcb_injected_type) & vmcb_valid_event_inj_types_mask) )

If vmcb_injected_type really could take all possible uint8_t values (see
below), this shift would be at risk of becoming UB. And uint8_t is a
stronger hint that all possible values may appear than unsigned int is.

> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
> +               vmcb_injected_type);

... what purpose do they serve here (and below)?

As to the use of PRIx8: Imo that's unnecessary to use. We assume
sizeof(int) >= 4, and every type smaller than that will be promoted to
int. Just %#x will hence do here (and below), improving readability.
Furthermore, the use of fixed-width types here is in conflict with
./CODING_STYLE anyway. I'm willing to accept it for variables holding
vector numbers (albeit longer term they will apparently need to widen
anyway), but the other two should be unsigned int.

> +    if ( (vmcb_injected_type == X86_ET_HW_EXC) &&
> +         !is_valid_injected_exception_vector(vmcb, vmcb_injected_vector) )
> +        PRINTF("eventinj: Invalid exception type: (%#"PRIx8") vector: "
> +               "(%#"PRIx8") for the platform\n",

Does "for the platform" really add any value? With it dropped, the
whole format string could also go on a single line (which we generally
prefer).

One more check would likely be worthwhile doing: We have X86_EXC_HAVE_EC,
and vmcb->event_inj.ev could also do with checking.

Finally a more general comment: svm_vmcb_isvalid() is used solely out of
nestedsvm.c. I hence think it would better move there, and such moving
would better come ahead of adding more code (which would then also need
moving).

Jan