[Xen-devel] [PATCH 2/2] x86/hvm: Don't intercept #UD exceptions in general
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--- CC: Jan Beulich CC: Jun Nakajima CC: Kevin Tian CC: Boris Ostrovsky CC: Suravee Suthikulpanit CC: Aravind Gopalakrishnan --- 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; } @@ -707,6 +719,12 @@ long arch_do_domctl( xen_domctl_cpuid_t *ctl = >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 = >arch.cpuids[i]; diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 674feea..7a15d49 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -93,12 +93,10 @@ unsigned long __section(".bss.page_aligned") static bool_t __initdata opt_hap_enabled = 1; boolean_param("hap", opt_hap_enabled); -#ifndef NDEBUG +#ifndef opt_hvm_fep /* Permit use of the Forced Emulation Prefix in HVM guests */ -static bool_t opt_hvm_fep; +bool_t opt_hvm_fep; boolean_param("hvm_fep", opt_hvm_fep); -#else -#define opt_hvm_fep 0 #endif /* 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 = >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); +} + static void svm_sync_vmcb(struct vcpu *v) { struct arch_svm_struct *arch_svm = >arch.hvm_svm; @@ -2245,6 +2257,7 @@ static struct hvm_function_table __initdata svm_function_table = { .get_shadow_gs_base = svm_get_shadow_gs_base, .update_guest_cr = svm_update_guest_cr, .update_guest_efer= svm_update_guest_efer, +.update_guest_vendor = svm_update_guest_vendor, .set_guest_pat= svm_set_guest_pat, .get_guest_pat= svm_get_guest_pat, .set_tsc_offset = svm_set_tsc_offset, diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c index 9ea014f..be2dc32 100644 --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@
Re: [Xen-devel] [PATCH 2/2] x86/hvm: Don't intercept #UD exceptions in general
On 01/27/2016 01:59 PM, Andrew Cooper wrote: On 27/01/16 18:49, Boris Ostrovsky wrote: On 01/27/2016 01:11 PM, Andrew Cooper wrote: 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. Is it safe to unpause a domain here if it is not running? -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/hvm: Don't intercept #UD exceptions in general
On 27/01/16 19:14, Boris Ostrovsky wrote: > On 01/27/2016 01:59 PM, Andrew Cooper wrote: >> On 27/01/16 18:49, Boris Ostrovsky wrote: >>> On 01/27/2016 01:11 PM, Andrew Cooper wrote: 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. > > Is it safe to unpause a domain here if it is not running? Yes - all pausing/unpausing is reference counted, including the initial systemcontroller pause reference taken (on behalf of the toolstack domain) during the createdomain hypercall. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/hvm: Don't intercept #UD exceptions in general
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--- CC: Jan Beulich CC: Jun Nakajima CC: Kevin Tian CC: Boris Ostrovsky CC: Suravee Suthikulpanit CC: Aravind Gopalakrishnan --- 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? @@ -707,6 +719,12 @@ long arch_do_domctl( xen_domctl_cpuid_t *ctl = >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 = >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 = >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). -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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>> --- >> CC: Jan Beulich >> CC: Jun Nakajima >> CC: Kevin Tian >> CC: Boris Ostrovsky >> CC: Suravee Suthikulpanit >> CC: Aravind Gopalakrishnan >> --- >> 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 = >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 = >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 = >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@lists.xen.org http://lists.xen.org/xen-devel