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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 16/07/2026 à 17:41, 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>
> ---
> Changes in v2:
> - Remove the redundant SVM_EVENT_INJ_TYPE_MASK and SVM_EVENT_INJ_VEC_MASK
>    constants.
> - Correct the Injected Event Type consistency check to disallow the injection
>    of reserved type 1 events.
> ---
> 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/2682300446
> ---
>   xen/arch/x86/hvm/svm/vmcb.c | 40 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 975a1eaef8..c31d2a6f58 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) ||
This particular exception is special. AMD APM states that this event is 
"impossible" if the guest is in 64-bit mode and will cause 
VMEXIT_INVALID in such case.
> 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 and no guest state instructions will be 
executed; VMRUN will immediately exit with an error code of VMEXIT_INVALID.
So this one likely want a additional check for hvm_guest_x86_mode() != 
X86_MODE_64BIT.
It looks like #OF has the same quirk (invalid in 64-bits mode).

Though I don't know if any other exception has a similar behavior though.

> +             (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 think using a switch here would help making things more readable, 
especially if we need to add additional comparisons in specific cases 
(SEV-ES for #VC, !64-bits for #BR and #OF, ...).
I have in mind something like

  switch (vmcb_injected_vector)
  {
  case X86_EXC_OF:
  case X86_EXC_BR:
      return hvm_guest_x86_mode(v) != X86_MODE_64BIT;

  (all other special cases, ...)

  case X86_EXC_UD:
  (all other simple cases ...)
      return true;

  default:
      return false;
  }

> +
>   bool svm_vmcb_isvalid(
>       const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v,
>       bool verbose)
> @@ -330,6 +355,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);
>   
>   #define PRINTF(fmt, args...) do { \
>       if ( !verbose ) return true; \
> @@ -392,6 +423,15 @@ 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) )
> +        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;
>   }
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature