[v2] x86/vPMU: constrain MSR_IA32_DS_AREA loads

Commit Message

Jan Beulich Dec. 18, 2015, 4:51 p.m. UTC

For one, loading the MSR with a possibly non-canonical address was
possible since the verification is conditional, while the MSR load
wasn't. And then for PV guests we need to further limit the range of
valid addresses to exclude the hypervisor range.

Signed-off-by: Jan Beulich <[e-mail]>
---
v2: Also alter the MSR write check, in anticipation of it becoming
    accessible by PV guests eventually.
x86/vPMU: constrain MSR_IA32_DS_AREA loads

For one, loading the MSR with a possibly non-canonical address was
possible since the verification is conditional, while the MSR load
wasn't. And then for PV guests we need to further limit the range of
valid addresses to exclude the hypervisor range.

Signed-off-by: Jan Beulich <[e-mail]>
---
v2: Also alter the MSR write check, in anticipation of it becoming
    accessible by PV guests eventually.

--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -366,7 +366,8 @@ static inline void __core2_vpmu_load(str
     }
 
     wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
-    wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
+    if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
+        wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
     wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
 
     if ( !has_hvm_container_vcpu(v) )
@@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
             enabled_cntrs |= (1ULL << i);
     }
 
-    if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
-         !is_canonical_address(core2_vpmu_cxt->ds_area) )
+    if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
+         !(has_hvm_container_vcpu(v)
+           ? is_canonical_address(core2_vpmu_cxt->ds_area)
+           : __addr_ok(core2_vpmu_cxt->ds_area)) )
         return -EINVAL;
 
     if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) ||
@@ -609,7 +612,9 @@ static int core2_vpmu_do_wrmsr(unsigned
     case MSR_IA32_DS_AREA:
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
         {
-            if ( !is_canonical_address(msr_content) )
+            if ( !(has_hvm_container_vcpu(v)
+                   ? is_canonical_address(msr_content)
+                   : __addr_ok(msr_content)) )
             {
                 gdprintk(XENLOG_WARNING,
                          "Illegal address for IA32_DS_AREA: %#" PRIx64 "x\n",

Comments

Boris Ostrovsky Dec. 18, 2015, 4:59 p.m. UTC | #1

On 12/18/2015 11:51 AM, Jan Beulich wrote:
> For one, loading the MSR with a possibly non-canonical address was
> possible since the verification is conditional, while the MSR load
> wasn't. And then for PV guests we need to further limit the range of
> valid addresses to exclude the hypervisor range.
>
> Signed-off-by: Jan Beulich <[e-mail]>
> ---
> v2: Also alter the MSR write check, in anticipation of it becoming
>      accessible by PV guests eventually.

Reviewed-by: Boris Ostrovsky <[e-mail]>

Tian, Kevin Dec. 20, 2015, 7:14 a.m. UTC | #2

> From: Jan Beulich [mailto:[e-mail]]
> Sent: Saturday, December 19, 2015 12:52 AM
> 
> For one, loading the MSR with a possibly non-canonical address was
> possible since the verification is conditional, while the MSR load
> wasn't. And then for PV guests we need to further limit the range of
> valid addresses to exclude the hypervisor range.
> 
> Signed-off-by: Jan Beulich <[e-mail]>

Acked-by: Kevin Tian <[e-mail]>

Patch

--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -366,7 +366,8 @@  static inline void __core2_vpmu_load(str
     }
 
     wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
-    wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
+    if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
+        wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
     wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
 
     if ( !has_hvm_container_vcpu(v) )
@@ -415,8 +416,10 @@  static int core2_vpmu_verify(struct vcpu
             enabled_cntrs |= (1ULL << i);
     }
 
-    if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
-         !is_canonical_address(core2_vpmu_cxt->ds_area) )
+    if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
+         !(has_hvm_container_vcpu(v)
+           ? is_canonical_address(core2_vpmu_cxt->ds_area)
+           : __addr_ok(core2_vpmu_cxt->ds_area)) )
         return -EINVAL;
 
     if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) ||
@@ -609,7 +612,9 @@  static int core2_vpmu_do_wrmsr(unsigned
     case MSR_IA32_DS_AREA:
         if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) )
         {
-            if ( !is_canonical_address(msr_content) )
+            if ( !(has_hvm_container_vcpu(v)
+                   ? is_canonical_address(msr_content)
+                   : __addr_ok(msr_content)) )
             {
                 gdprintk(XENLOG_WARNING,
                          "Illegal address for IA32_DS_AREA: %#" PRIx64 "x\n",