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

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

[Abdelkareem Abdelsaamad]

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@citrix.com/

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.

Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@citrix.com>
---
Changes in v4:
- Reject the injected events with the valid bit not set.
- Fix the APM revision details.
- Rename the is_valid_svm_vmcb_injected_exception_vector to the shorter
  is_valid_injected_exception_vector.
- Address coding style comments regarding !! operator usage for boolean
  returns, concise debug messaging for reserved vectors, and blank lines
  between non-fall-through case blocks.
- Drop the X86_EXC_HV from the permitted vectors.

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 guests with enabled CET
  to prevent VMRUN failures on hardware without CET support.

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
            exception type: (0x3) vector: (0x2) for the platform. 
   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.
          On the Genoa host, VMRUN immediately triggers VMEXIT_VMMCALL.
      With the changes included:
          On the Naples host and the Genoa host, VMEXIT_INVALID is reported back
          without VMRUN execution.

 - CI tests:
https://gitlab.com/xen-project/people/aabdelsa/xen/-/pipelines/2783297011
---
 xen/arch/x86/hvm/svm/vmcb.c | 58 +++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 975a1eaef8..f983179dc8 100644
--- 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:
+    case X86_EXC_XM:
+    case X86_EXC_SX:
+        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;
+
+    default:
+        return false;
+    }
+}
+
 bool svm_vmcb_isvalid(
     const char *from, const struct vmcb_struct *vmcb, const struct vcpu *v,
     bool verbose)
@@ -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);
 
 #define PRINTF(fmt, args...) do { \
     if ( !verbose ) return true; \
@@ -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);
+
+    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_injected_exception_vector(vmcb, vmcb_injected_vector) )
+        PRINTF("eventinj: Invalid exception type: (%#"PRIx8") vector: "
+               "(%#"PRIx8") for the platform\n",
+               vmcb_injected_type, vmcb_injected_vector);
+
 #undef PRINTF
     return ret;
 }
-- 
2.53.0

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

[Jan Beulich]

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@citrix.com/
> 
> 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

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

[Abdelkareem Abdelsaamad]

On 26.08.2026 15:33, Jan Beulich wrote:
>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@citrix.com/
>> 
>> 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.
I will put it separately with a comment in v5.
Regarding the APM question: the manual does not appear to fully document
generation-specific or microarchitecture-dependent constraints for all the
vectors. Some vectors are only valid starting with the specific CPU generation
that introduced the corresponding feature. To verify the hardware behavior, I
am performing case-by-case testing on 64-bit Windows guests. I inject the
various events at the end of svm_vmexit_handler and see the result:
 - If it triggers VMEXIT_INVALID, the injection is invalid.
 - If it triggers a triple fault, the event passed the hardware checks and 
 completed the event delivery. It is valid.
I am testing this across a Naples (older) and a Genoa (modern) platform. For
the X86_EXC_HV, I am consistently getting VMEXIT_INVALID. I will update the
code comments in v5 with the testing matrix used and the findings.
>
>> --- 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?
The testing I performed (see previous comment) does not show that set CR4.MCE
is required for the valid injection.
>> +    case X86_EXC_XM:
>
>As before: Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?
The testing I performed (see previous comment) does not show that set CR4.MCE
is required for the valid injection.
>> +    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 ...
The testing I performed (see the first comment) does not show that set CR4.MCE
is required for the valid injection.
>
>> +        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.
In my testing, the hardware behavior differs across the generations support for
the Control-flow Enforcement Technology (CET):
 - Naples (No hardware support): Injecting the event when the feature is
   completely unsupported by the CPU results in VMEXIT_INVALID. The VMCB's CR4
   bit is not set as it is expected.
 - Genoa (Hardware support exists): If the CPU supports the feature but the
   guest has not enabled it in CR4 (not opted-in), injecting the event
   results in a triple fault. I am accordingly checking for the CPU feature and
   report it as invalid.
>
>> @@ -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?
I thought the mask varibale name is sufficient? I am also fine with the #define
but it will be a one time usage. What is your suggestion?
>> @@ -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.
I am OK to change to unsigned int for safety with future possible extensions.
>
>> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
>> +               vmcb_injected_type);
>
>... what purpose do they serve here (and below)?
I did it just to go with the previous format but I am OK to drop in v5.
>
>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.
I will change to %#x and I will change the other two variables to 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).
My point was to give an expressive hint as the injection combination can be
invalid on one platform/mode but valid on another (for example, #BR injection
triggers VMEXIT_INVALID in 64-bit long mode but is allowed in 32-bit mode). The
CPU generation also matter as outlined before.
>
>One more check would likely be worthwhile doing: We have X86_EXC_HAVE_EC,
>and vmcb->event_inj.ev could also do with checking.
I will test this and add the check in v5 in the case it is needed.
>
>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).
This is a point that needs to reach a consensus between you and Teddy. In the
v3 review, Teddy commented to expand the use of svm_vmcb_isvalid() by calling
it within svm_vmexit_handler() when exit_reason == VMEXIT_INVALID. If I follow
Teddy's suggestion, the function needs to stay in a shared location. Otherwise,
the function will be kept, as is, strictly for nested SVM usage and moving it
>Jan

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

[Abdelkareem Abdelsaamad]

>>> @@ -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?
>The testing I performed (see previous comment) does not show that set CR4.MCE
>is required for the valid injection.
>>> +    case X86_EXC_XM:
>>
>>As before: Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?
>The testing I performed (see previous comment) does not show that set CR4.MCE
>is required for the valid injection.
I meant ..does not show that set CR4.OSXMMEXCPT is required.
>>
>>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 ...
>The testing I performed (see the first comment) does not show that set CR4.MCE
>is required for the valid injection.
I meant ..does not show that any CR4 bit is required.

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

[Jan Beulich]

On 06.09.2026 15:22, Abdelkareem Abdelsaamad wrote:
>>>> @@ -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?
>> The testing I performed (see previous comment) does not show that set CR4.MCE
>> is required for the valid injection.
>>>> +    case X86_EXC_XM:
>>>
>>> As before: Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?
>> The testing I performed (see previous comment) does not show that set CR4.MCE
>> is required for the valid injection.
> I meant ..does not show that set CR4.OSXMMEXCPT is required.
>>>
>>> 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 ...
>> The testing I performed (see the first comment) does not show that set CR4.MCE
>> is required for the valid injection.
> I meant ..does not show that any CR4 bit is required.

And I didn't mention CR4. I intentionally said "without any constraints".

Jan

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

[Jan Beulich]

On 06.09.2026 15:11, Abdelkareem Abdelsaamad wrote:
> On 26.08.2026 15:33, Jan Beulich wrote:
>> 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@citrix.com/
>>>
>>> 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.
> I will put it separately with a comment in v5.
> Regarding the APM question: the manual does not appear to fully document
> generation-specific or microarchitecture-dependent constraints for all the
> vectors.

And there was no request to add conditionals going in that direction. Checks
want to be against feature or enable bits, wherever possible.

> Some vectors are only valid starting with the specific CPU generation
> that introduced the corresponding feature. To verify the hardware behavior, I
> am performing case-by-case testing on 64-bit Windows guests. I inject the
> various events at the end of svm_vmexit_handler and see the result:
>  - If it triggers VMEXIT_INVALID, the injection is invalid.
>  - If it triggers a triple fault, the event passed the hardware checks and 
>  completed the event delivery. It is valid.
> I am testing this across a Naples (older) and a Genoa (modern) platform. For
> the X86_EXC_HV, I am consistently getting VMEXIT_INVALID. I will update the
> code comments in v5 with the testing matrix used and the findings.
>>
>>> --- 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?
> The testing I performed (see previous comment) does not show that set CR4.MCE
> is required for the valid injection.

I find this worrying. Roger, any chance you could try to find out whether
that's perhaps more an erratum than intended behavior?

>>> +        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.
> In my testing, the hardware behavior differs across the generations support for
> the Control-flow Enforcement Technology (CET):
>  - Naples (No hardware support): Injecting the event when the feature is
>    completely unsupported by the CPU results in VMEXIT_INVALID. The VMCB's CR4
>    bit is not set as it is expected.
>  - Genoa (Hardware support exists): If the CPU supports the feature but the
>    guest has not enabled it in CR4 (not opted-in), injecting the event
>    results in a triple fault. I am accordingly checking for the CPU feature and
>    report it as invalid.

A guest triple fault, I assume? I'm not entirely convinced this is a sufficient
indication of injection being permitted, even though I agree it very much looks
so. Then again, like above, I'm also unconvinced this is actually intended
behavior. Guests unaware of a feature (and hence not enabling it) should never
observe exceptions related to only that feature.

>>> @@ -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?
> I thought the mask varibale name is sufficient? I am also fine with the #define
> but it will be a one time usage. What is your suggestion?

I gave my suggestion. The variable name I, personally, consider too long
anyway.

>>> @@ -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.
> I am OK to change to unsigned int for safety with future possible extensions.
>>
>>> +        PRINTF("eventinj: Invalid Injected Event Type: (%#"PRIx8")\n",
>>> +               vmcb_injected_type);
>>
>> ... what purpose do they serve here (and below)?
> I did it just to go with the previous format but I am OK to drop in v5.

You did notice the difference in message type, though? Where parentheses
are used in existing messages, the values put there serve as auxiliary
information to the wording used. Whereas here you plainly dump a value,
without saying what exactly is wrong (that's actually said by the value
dumped).

>> +    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).
> My point was to give an expressive hint as the injection combination can be
> invalid on one platform/mode but valid on another (for example, #BR injection
> triggers VMEXIT_INVALID in 64-bit long mode but is allowed in 32-bit mode). The
> CPU generation also matter as outlined before.

I fear I don't see how this explains the extra words used.

>> 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).
> This is a point that needs to reach a consensus between you and Teddy. In the
> v3 review, Teddy commented to expand the use of svm_vmcb_isvalid() by calling
> it within svm_vmexit_handler() when exit_reason == VMEXIT_INVALID. If I follow
> Teddy's suggestion, the function needs to stay in a shared location. Otherwise,
> the function will be kept, as is, strictly for nested SVM usage and moving it

Well, my comment was based on present code. If you follow Teddy's suggestion,
my comment would simply become inapplicable.

Jan

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

[Jan Beulich]

On 21.09.2026 17:50, Abdelkareem Abdelsaamad wrote:
> On 07.09.2026 08:50, Jan Beulich wrote:
>> On 06.09.2026 15:11, Abdelkareem Abdelsaamad wrote:
>>> On 26.08.2026 15:33, Jan Beulich wrote:
>>>> On 23.08.2026 18:11, Abdelkareem Abdelsaamad wrote:
>>>>> +    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.
>>> In my testing, the hardware behavior differs across the generations support for
>>> the Control-flow Enforcement Technology (CET):
>>>  - Naples (No hardware support): Injecting the event when the feature is
>>>    completely unsupported by the CPU results in VMEXIT_INVALID. The VMCB's CR4
>>>    bit is not set as it is expected.
>>>  - Genoa (Hardware support exists): If the CPU supports the feature but the
>>>    guest has not enabled it in CR4 (not opted-in), injecting the event
>>>    results in a triple fault. I am accordingly checking for the CPU feature and
>>>    report it as invalid.
>>
>> A guest triple fault, I assume?
> Yes, that is correct. I meant a guest triple fault.
>> I'm not entirely convinced this is a sufficient
>> indication of injection being permitted, even though I agree it very much looks
>> so. Then again, like above, I'm also unconvinced this is actually intended
>> behavior. Guests unaware of a feature (and hence not enabling it) should never
>> observe exceptions related to only that feature.
> The testing, I performed shows the following behavior across the CPU
> generations:
>  - On CPUU generations that support the feature (e.g., Genoa supporting 
>    Control-flow Enforcement Technology / CET), the injection results in
>    a guest triple fault. If the the guest did not opt-in for the CET feature.
>    No VMEXIT_INVALID results by the injection.
>  - On older hardware generations that completely lack the feature (e.g., Naples), 
>    the injection immediately results in a VMEXIT_INVALID.
> 
> The current hardware behavior seems to depend on whether the underlying
> physical CPU understands the feature, rather than whether the guest has opted
> into it via CR4. The patch expands this to consider the injection will result
> in VMEXIT_INVALID if the guest did not opt into the feature.
> 
> Are you suggesting to reather explicitly check the CPU model/generation instead
> of checking X86_CR4_CET? For example, allowing the injection on Genoa platforms
> regardless of whether the guest has enabled the CET capability? I am concerned 
> that handling this via CPU model checks might introduce architectural edge
> cases and/or add maintenance complication—what are your thoughts on that
> approach?

No, I'm not suggesting to go by CPU model. That would be wrong in certain
migration scenarios, afaict.

Jan

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

[Abdelkareem Abdelsaamad]

On 07.09.2026 08:36, Jan Beulich wrote:
>On 06.09.2026 15:22, Abdelkareem Abdelsaamad wrote:
>>>>> @@ -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?
>>> The testing I performed (see previous comment) does not show that set CR4.MCE
>>> is required for the valid injection.
>>>>> +    case X86_EXC_XM:
>>>>
>>>> As before: Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?
>>> The testing I performed (see previous comment) does not show that set CR4.MCE
>>> is required for the valid injection.
>> I meant ..does not show that set CR4.OSXMMEXCPT is required.
>>>>
>>>> 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 ...
>>> The testing I performed (see the first comment) does not show that set CR4.MCE
>>> is required for the valid injection.
>> I meant ..does not show that any CR4 bit is required.
>
>And I didn't mention CR4. I intentionally said "without any constraints".
I believe the Security Exception (Vector 30) is architecturally valid on AMD
Naples (EPYC 7001) and Rome (EPYC 7002) platforms. Event injection of vector 30
then does not require specific guest enablement I am aware of.

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

[Jan Beulich]

On 21.09.2026 19:07, Abdelkareem Abdelsaamad wrote:
> On 07.09.2026 08:36, Jan Beulich wrote:
>> On 06.09.2026 15:22, Abdelkareem Abdelsaamad wrote:
>>>>>> @@ -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?
>>>> The testing I performed (see previous comment) does not show that set CR4.MCE
>>>> is required for the valid injection.
>>>>>> +    case X86_EXC_XM:
>>>>>
>>>>> As before: Doesn't #XM (AMD: #XF) require CR4.OSXMMEXCPT to be set?
>>>> The testing I performed (see previous comment) does not show that set CR4.MCE
>>>> is required for the valid injection.
>>> I meant ..does not show that set CR4.OSXMMEXCPT is required.
>>>>>
>>>>> 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 ...
>>>> The testing I performed (see the first comment) does not show that set CR4.MCE
>>>> is required for the valid injection.
>>> I meant ..does not show that any CR4 bit is required.
>>
>> And I didn't mention CR4. I intentionally said "without any constraints".
> I believe the Security Exception (Vector 30) is architecturally valid on AMD
> Naples (EPYC 7001) and Rome (EPYC 7002) platforms. Event injection of vector 30
> then does not require specific guest enablement I am aware of.
IOW another one of those cases where an unaware guest can be sent an exception
which may end up killing that guest? Hmm... Not your fault of course, yet still
problematic. May want making explicit in the description (maybe even a code
comment) then, I think.

Jan