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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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.
> 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