[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] x86/hvm: Don't intercept #UD exceptions in general
On 27/01/16 18:49, Boris Ostrovsky wrote: > On 01/27/2016 01:11 PM, Andrew Cooper wrote: >> c/s 0f1cb96e "x86 hvm: Allow cross-vendor migration" caused HVM >> domains to >> unconditionally intercept #UD exceptions. While cross-vendor >> migration is >> cool as a demo, it is extremely niche. >> >> Intercepting #UD allows userspace code in a multi-vcpu guest to execute >> arbitrary instructions in the x86 emulator by having one thread >> execute a ud2a >> instruction, and having a second thread rewrite the instruction >> before the >> emulator performs an instruction fetch. >> >> XSAs 105, 106 and 110 are all examples where guest userspace can use >> bugs in >> the x86 emulator to compromise security of the domain, either by >> privilege >> escalation or causing a crash. >> >> c/s 2d67a7a4 "x86: synchronize PCI config space access decoding" >> introduced (amongst other things) a per-domain vendor, based on the >> guests >> cpuid policy. >> >> Use the per-guest vendor to enable #UD interception only when a >> domain is >> configured for a vendor different to the current hardware. (#UD >> interception >> is also enabled if hvm_fep is specified on the Xen command line. >> This is a >> debug-only option whose entire purpose is for testing the x86 emulator.) >> >> As a result, the overwhelming majority of usecases now have #UD >> interception >> disabled, removing an attack surface for malicious guest userspace. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> --- >> CC: Jan Beulich <JBeulich@xxxxxxxx> >> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx> >> CC: Kevin Tian <kevin.tian@xxxxxxxxx> >> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> >> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> >> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx> >> --- >> xen/arch/x86/domctl.c | 18 ++++++++++++++++++ >> xen/arch/x86/hvm/hvm.c | 6 ++---- >> xen/arch/x86/hvm/svm/svm.c | 13 +++++++++++++ >> xen/arch/x86/hvm/svm/vmcb.c | 1 + >> xen/arch/x86/hvm/vmx/vmcs.c | 1 + >> xen/arch/x86/hvm/vmx/vmx.c | 15 +++++++++++++++ >> xen/include/asm-x86/hvm/hvm.h | 15 ++++++++++++++- >> 7 files changed, 64 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c >> index 1d71216..1084e82 100644 >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -65,8 +65,20 @@ static void update_domain_cpuid_info(struct domain >> *d, >> .ecx = ctl->ecx >> } >> }; >> + int old_vendor = d->arch.x86_vendor; >> d->arch.x86_vendor = get_cpu_vendor(vendor_id.str, >> gcv_guest); >> + >> + if ( is_hvm_domain(d) && (d->arch.x86_vendor != old_vendor) ) >> + { >> + struct vcpu *v; >> + >> + domain_pause(d); >> + for_each_vcpu( d, v ) >> + hvm_update_guest_vendor(v); >> + domain_unpause(d); >> + } >> + >> break; >> } > > Not specific to this patch, but shouldn't we pause/unpause domain for > the whole routine? Not specifically, although that might be better lonterm. In practice, this hypercall is only made as part of domain construction, and never at domain runtime. > > >> @@ -707,6 +719,12 @@ long arch_do_domctl( >> xen_domctl_cpuid_t *ctl = &domctl->u.cpuid; >> cpuid_input_t *cpuid, *unused = NULL; >> + if ( d == currd ) /* no domain_pause() */ >> + { >> + ret = -EINVAL; >> + break; >> + } >> + >> for ( i = 0; i < MAX_CPUID_INPUT; i++ ) >> { >> cpuid = &d->arch.cpuids[i]; > > ... > >> /* Xen command-line option to enable altp2m */ >> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c >> index 953e0b5..44a1250 100644 >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -597,6 +597,18 @@ static void svm_update_guest_efer(struct vcpu *v) >> vmcb_set_efer(vmcb, new_efer); >> } >> +static void svm_update_guest_vendor(struct vcpu *v) >> +{ >> + struct arch_svm_struct *arch_svm = &v->arch.hvm_svm; >> + struct vmcb_struct *vmcb = arch_svm->vmcb; >> + >> + if ( opt_hvm_fep || >> + (v->domain->arch.x86_vendor != boot_cpu_data.x86_vendor) ) >> + vmcb->_exception_intercepts |= (1U << TRAP_invalid_op); >> + else >> + vmcb->_exception_intercepts &= ~(1U << TRAP_invalid_op); >> +} > > I think you need to clear clean bits here (at least bit 0). Hmm - looks like I copied some other code in need of fixing. I will see what I can do. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |