From 0653a0598613ea9d7598ad8f9ace89ed7498410c Mon Sep 17 00:00:00 2001 From: Andrew Cooper Date: Mon, 30 Jul 2018 11:01:28 +0100 Subject: [PATCH] x86/vtx: Fix the checking for unknown/invalid MSR_DEBUGCTL bits The VPMU_MODE_OFF early-exit in vpmu_do_wrmsr() introduced by c/s 11fe998e56 bypasses all reserved bit checking in the general case. As a result, a guest can enable BTS when it shouldn't be permitted to, and lock up the entire host. With vPMU active (not a security supported configuration, but useful for debugging), the reserved bit checking in broken, caused by the original BTS changeset 1a8aa75ed. From a correctness standpoint, it is not possible to have two different pieces of code responsible for different parts of value checking, if there isn't an accumulation of bits which have been checked. A practical upshot of this is that a guest can set any value it wishes (usually resulting in a vmentry failure for bad guest state). Therefore, fix this by implementing all the reserved bit checking in the main MSR_DEBUGCTL block, and removing all handling of DEBUGCTL from the vPMU MSR logic. This is XSA-269 Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich --- xen/arch/x86/cpu/vpmu_intel.c | 20 -------------------- xen/arch/x86/hvm/vmx/vmx.c | 27 ++++++++++++++++++++------- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c index 207e2e712c..d4444f0d94 100644 --- a/xen/arch/x86/cpu/vpmu_intel.c +++ b/xen/arch/x86/cpu/vpmu_intel.c @@ -535,27 +535,7 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t *enabled_cntrs; if ( !core2_vpmu_msr_common_check(msr, &type, &index) ) - { - /* Special handling for BTS */ - if ( msr == MSR_IA32_DEBUGCTLMSR ) - { - supported |= IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS | - IA32_DEBUGCTLMSR_BTINT; - - if ( cpu_has(¤t_cpu_data, X86_FEATURE_DSCPL) ) - supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS | - IA32_DEBUGCTLMSR_BTS_OFF_USR; - if ( !(msr_content & ~supported) && - vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) - return 0; - if ( (msr_content & supported) && - !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) - printk(XENLOG_G_WARNING - "%pv: Debug Store unsupported on this CPU\n", - current); - } return -EINVAL; - } ASSERT(!supported); diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 48d3c54e84..2c490cf62a 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3097,11 +3097,14 @@ void vmx_vlapic_msr_changed(struct vcpu *v) static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) { struct vcpu *v = current; + const struct cpuid_policy *cp = v->domain->arch.cpuid; HVM_DBG_LOG(DBG_LEVEL_MSR, "ecx=%#x, msr_value=%#"PRIx64, msr, msr_content); switch ( msr ) { + uint64_t rsvd; + case MSR_IA32_SYSENTER_CS: __vmwrite(GUEST_SYSENTER_CS, msr_content); break; @@ -3117,16 +3120,26 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content) break; case MSR_IA32_DEBUGCTLMSR: { int i, rc = 0; - uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF; - if ( boot_cpu_has(X86_FEATURE_RTM) ) - supported |= IA32_DEBUGCTLMSR_RTM; - if ( msr_content & ~supported ) + rsvd = ~(IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF); + + /* TODO: Wire vPMU settings properly through the CPUID policy */ + if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_BTS) ) { - /* Perhaps some other bits are supported in vpmu. */ - if ( vpmu_do_wrmsr(msr, msr_content, supported) ) - break; + rsvd &= ~(IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS | + IA32_DEBUGCTLMSR_BTINT); + + if ( cpu_has(¤t_cpu_data, X86_FEATURE_DSCPL) ) + rsvd &= ~(IA32_DEBUGCTLMSR_BTS_OFF_OS | + IA32_DEBUGCTLMSR_BTS_OFF_USR); } + + if ( cp->feat.rtm ) + rsvd &= ~IA32_DEBUGCTLMSR_RTM; + + if ( msr_content & rsvd ) + goto gp_fault; + if ( msr_content & IA32_DEBUGCTLMSR_LBR ) { const struct lbr_info *lbr = last_branch_msr_get(); -- 2.18.0