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 <[email protected]> > --- > CC: Jan Beulich <[email protected]> > CC: Roger Pau Monné <[email protected]> > --- > 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
