Re: [PATCH] x86/hvm: Unilaterally inject #UD for unknown VMExits

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Fri Nov 28, 2025 at 6:47 PM CET, Andrew Cooper wrote:
> While we do this for unknown user mode exits, crashing for supervisor mode
> exits is unhelpful.  Intel in particular expect the unknown case to be #UD
> because they do introduce new instructions with new VMEXIT_* codes without
> other enablement controls.  e.g. MSRLIST, USER_MSR, MSR_IMM, but AMD have
> RDPRU and SKINIT as examples too.

I don't know how often Intel adds intercepts (or whatever the VMX equivalent is)
without default-off knobs, but there's a potentially dangerous assumption here
about all intercepts being synchronous with the executed instruction. Some might
depend on other events (i.e: NMIs, IRQs, IPIs, etc) and injecting #UD in those
cases would be very insecure for the guest. It might encourage the kernel to
interpret the current instruction that the kernel can't know wasn't meant to
ever trigger #UD. This would be an integrity-compromising mistake to make.

IOW, I think this is a dangerous default to have and Xen should just crash the
domain irrespective of CPL. At least on SVM. If a guest executes SKINIT and it
doesn't exist 

>
> A guest which gets its CPUID checks wrong can trigger the VMExit, and the
> VMexit handler would need to emulate the CPUID check and #UD anyway.  This
> would be a guest bug, and giving it an exception is the right course of
> action.

You you need a guest bug compounded with an outdated hypervisor to reproduce the
crash, and even then it's perfectly benign. Beats the alternative described
above, IMO.

>
> Drop the "#UD or crash" semantics and make it exclusively #UD.  Rename the
> lables to match the changed semantics.  Fold the cases which were plain #UD's
> too.

nit: typo s/lables/labels

Also, does why emacs double spaces after each sentence on reflow? You have them
in a number of places.

>
> One case that was wrong beforehand was EPT_MISCONFIG.  Failing to resolve is
> always a Xen bug, and not even necesserily due to the instruction under %rip.
> Convert it to be an unconditional domain_crash() with applicable diagnostic
> information.

One example of the above synchronous vs asynchronous events.

>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 29 +++++++++--------------------
>  xen/arch/x86/hvm/vmx/vmx.c | 26 +++++++++++---------------
>  2 files changed, 20 insertions(+), 35 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 2d7c598ffe99..637b47084c25 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -66,15 +66,6 @@ static bool amd_erratum383_found __read_mostly;
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> -/* Only crash the guest if the problem originates in kernel mode. */
> -static void svm_crash_or_fault(struct vcpu *v)
> -{
> -    if ( vmcb_get_cpl(v->arch.hvm.svm.vmcb) )
> -        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> -    else
> -        domain_crash(v->domain);
> -}
> -
>  void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
>  {
>      struct vcpu *curr = current;
> @@ -85,7 +76,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, 
> unsigned int inst_len)
>      if ( unlikely(inst_len > MAX_INST_LEN) )
>      {
>          gdprintk(XENLOG_ERR, "Bad instruction length %u\n", inst_len);
> -        svm_crash_or_fault(curr);
> +        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);

This seems more than fair enough.

>          return;
>      }
>  
> @@ -2872,7 +2863,7 @@ void asmlinkage svm_vmexit_handler(void)
>           * task switch.  Decode under %rip to find its length.
>           */
>          if ( insn_len < 0 && (insn_len = svm_get_task_switch_insn_len()) == 
> 0 )
> -            goto crash_or_fault;
> +            goto inject_ud;

And so does this (semantically).

>  
>          hvm_task_switch(vmcb->ei.task_switch.sel,
>                          vmcb->ei.task_switch.iret ? TSW_iret :
> @@ -2984,13 +2975,6 @@ void asmlinkage svm_vmexit_handler(void)
>          svm_vmexit_do_rdtsc(regs, exit_reason == VMEXIT_RDTSCP);
>          break;
>  
> -    case VMEXIT_MONITOR:
> -    case VMEXIT_MWAIT:
> -    case VMEXIT_SKINIT:
> -    case VMEXIT_RDPRU:
> -        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> -        break;
> -
>      case VMEXIT_VMRUN:
>          svm_vmexit_do_vmrun(regs, v, regs->rax);
>          break;
> @@ -3083,8 +3067,13 @@ void asmlinkage svm_vmexit_handler(void)
>          gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", "
>                  "exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n",
>                  exit_reason, vmcb->exitinfo1, vmcb->exitinfo2);
> -    crash_or_fault:
> -        svm_crash_or_fault(v);
> +        fallthrough;
> +    case VMEXIT_MONITOR:
> +    case VMEXIT_MWAIT:
> +    case VMEXIT_SKINIT:
> +    case VMEXIT_RDPRU:
> +    inject_ud:
> +        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);

I understand monitor, mwait, skinit and rdpru triggering #UD, but in the
default case (and we're also covering "default" out of context) injecting #UD
might be meaningless, plain wrong or highly misleading (e.g: imagine receiving
#UD on IRQ). VMEXITs happen due to optional or mandatory intercepts about
synchronous and asynchronous events. If Xen receives a VMEXIT it doesn't
understand the ideal would be to BUG(), because they don't just come out of thin
air. You're meant to enable the intercepts in the VMCB. For defensive purposes
a domain_crash() would be fine too, as it partly is now. I don't understand
the CPL distinction. The kernel might trigger highly dangerous and
integrity-compromising paths if it chooses to interpret #UD and it happens to to
be an externally triggered event rather than something related to the current
instruction.

(dropped VMX, because I don't know about it)

Cheers,
Alejandro