x86/pv: Don't have %cr4.fsgsbase active behind a guest kernels back

Currently, a 64bit PV guest can appear to set and clear FSGSBASE in %cr4, but
the bit remains set in hardware.  Therefore, the {RD,WR}{FS,GS}BASE are usable
even when the guest kernel believes that they are disabled.

The FSGSBASE feature isn't currently supported in Linux, and its context
switch path has some optimisations which rely on userspace being unable to use
the WR{FS,GS}BASE instructions.  Xen's current behaviour undermines this
expectation.

In 64bit PV guest context, always load the guest kernels setting of FSGSBASE
into %cr4.  This requires adjusting how Xen uses the {RD,WR}{FS,GS}BASE
instructions.

 * Delete the cpu_has_fsgsbase helper.  It is no longer safe, as users need to
   check %cr4 directly.
 * The raw __rd{fs,gs}base() helpers are only safe to use when %cr4.fsgsbase
   is set.  Comment this property.
 * The {rd,wr}{fs,gs}{base,shadow}() and read_msr() helpers are updated to use
   the current %cr4 value to determine which mechanism to use.
 * toggle_guest_mode() and save_segments() are update to avoid reading
   fs/gsbase if the values in hardware cannot be stale WRT struct vcpu.  A
   consequence of this is that the write_cr() path needs to cache the current
   bases, as subsequent context switches will skip saving the values.
 * write_cr4() is updated to ensure that the shadow %cr4.fsgsbase value is
   observed in a safe way WRT the hardware setting, if an interrupt happens to
   hit in the middle.
 * load_segments() is updated to use the VMLOAD optimisation if FSGSBASE is
   unavailable, even if only gs_shadow needs updating.  As a minor perf
   improvement, check cpu_has_svm first to short circuit a context-dependent
   conditional on Intel hardware.
 * pv_make_cr4() is updated for 64bit PV guests to use the guest kernels
   choice of FSGSBASE.

This is part of XSA-293.

Reported-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
master commit: eccc170053e46b4ab1d9e7485c09e210be15bbd7
master date: 2019-03-05 13:54:05 +0100

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8ddb12ccc6a81d2cbde01174ff5973ed9e9a2d4c..20c0bd02d1d008d419b2d86d6e0c58a0a00661e1 100644 (file)
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -431,6 +431,16 @@ unsigned long pv_make_cr4(const struct vcpu *v)
     if ( d->arch.vtsc || (v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_TSD) )
         cr4 |= X86_CR4_TSD;
 
+    /*
+     * The {RD,WR}{FS,GS}BASE are only useable in 64bit code segments.  While
+     * we must not have CR4.FSGSBASE set behind the back of a 64bit PV kernel,
+     * we do leave it set in 32bit PV context to speed up Xen's context switch
+     * path.
+     */
+    if ( !is_pv_32bit_domain(d) &&
+         !(v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_FSGSBASE) )
+        cr4 &= ~X86_CR4_FSGSBASE;
+
     return cr4;
 }
 
@@ -2012,7 +2022,8 @@ static void save_segments(struct vcpu *v)
     regs->fs = read_sreg(fs);
     regs->gs = read_sreg(gs);
 
-    if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) )
+    /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
+    if ( (read_cr4() & X86_CR4_FSGSBASE) && !is_pv_32bit_vcpu(v) )
     {
         v->arch.pv_vcpu.fs_base = __rdfsbase();
         if ( v->arch.flags & TF_kernel_mode )

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 0072de75f95f54056e54287c5acd7356819a6b57..9eca57acf7d0874ecc6798fe8df48bc06f37cc53 100644 (file)
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1442,7 +1442,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
 
-    if ( cpu_has_fsgsbase )
+    if ( boot_cpu_has(X86_FEATURE_FSGSBASE) )
         set_in_cr4(X86_CR4_FSGSBASE);
 
     if ( opt_invpcid && cpu_has_invpcid )

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6cbbf3ff2e575d7a941a09c0028a36c6607fa521..583936e9cbf96703cca1a5a137ae532771665d18 100644 (file)
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2722,6 +2722,17 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
         }
 
         case 4: /* Write CR4 */
+            /*
+             * If this write will disable FSGSBASE, refresh Xen's idea of the
+             * guest bases now that they can no longer change.
+             */
+            if ( (v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_FSGSBASE) &&
+                 !(*reg & X86_CR4_FSGSBASE) )
+            {
+                v->arch.pv_vcpu.fs_base = __rdfsbase();
+                v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
+            }
+
             v->arch.pv_vcpu.ctrlreg[4] = pv_fixup_guest_cr4(v, *reg);
             write_cr4(pv_make_cr4(v));
             ctxt_switch_levelling(v);
@@ -2993,13 +3004,14 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
         case MSR_FS_BASE:
             if ( is_pv_32bit_domain(currd) )
                 goto fail;
-            val = cpu_has_fsgsbase ? __rdfsbase() : v->arch.pv_vcpu.fs_base;
+            val = (read_cr4() & X86_CR4_FSGSBASE) ? __rdfsbase()
+                                                  : v->arch.pv_vcpu.fs_base;
             goto rdmsr_writeback;
         case MSR_GS_BASE:
             if ( is_pv_32bit_domain(currd) )
                 goto fail;
-            val = cpu_has_fsgsbase ? __rdgsbase()
-                                   : v->arch.pv_vcpu.gs_base_kernel;
+            val = (read_cr4() & X86_CR4_FSGSBASE) ? __rdgsbase()
+                                                  : v->arch.pv_vcpu.gs_base_kernel;
             goto rdmsr_writeback;
         case MSR_SHADOW_GS_BASE:
             if ( is_pv_32bit_domain(currd) )

diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 56ed156753ff9d344f41454047e5d56cd34c40bc..04cc60dd1f0c035229048c9bc7bfe42d708f82ee 100644 (file)
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -265,7 +265,9 @@ void toggle_guest_mode(struct vcpu *v)
 {
     if ( is_pv_32bit_vcpu(v) )
         return;
-    if ( cpu_has_fsgsbase )
+
+    /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
+    if ( read_cr4() & X86_CR4_FSGSBASE )
     {
         if ( v->arch.flags & TF_kernel_mode )
             v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();

diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 04d9e28e9b43e847796e4c88ef5517ddff12629c..f9c83352d415f621ad82f25cebe0d92ff65b4a1f 100644 (file)
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -72,7 +72,6 @@
 #define cpu_has_nx             boot_cpu_has(X86_FEATURE_NX)
 #define cpu_has_clflush                boot_cpu_has(X86_FEATURE_CLFLUSH)
 #define cpu_has_page1gb                boot_cpu_has(X86_FEATURE_PAGE1GB)
-#define cpu_has_fsgsbase       boot_cpu_has(X86_FEATURE_FSGSBASE)
 #define cpu_has_aperfmperf     boot_cpu_has(X86_FEATURE_APERFMPERF)
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
 #define cpu_has_invpcid         boot_cpu_has(X86_FEATURE_INVPCID)

diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 4b4c15668c3a658538eb0c1ad1b5eca4978570a3..252cbdcce6a97c09b89eea12c1a704e9b9dd6c2b 100644 (file)
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -93,6 +93,14 @@ static inline uint64_t rdtsc(void)
                          : "=a" (low), "=d" (high) \
                          : "c" (counter))
 
+/*
+ * On hardware supporting FSGSBASE, the value loaded into hardware is the
+ * guest kernel's choice for 64bit PV guests (Xen's choice for Idle, HVM and
+ * 32bit PV).
+ *
+ * Therefore, the {RD,WR}{FS,GS}BASE instructions are only safe to use if
+ * %cr4.fsgsbase is set.
+ */
 static inline unsigned long __rdfsbase(void)
 {
     unsigned long base;
@@ -123,7 +131,7 @@ static inline unsigned long rdfsbase(void)
 {
     unsigned long base;
 
-    if ( cpu_has_fsgsbase )
+    if ( read_cr4() & X86_CR4_FSGSBASE )
         return __rdfsbase();
 
     rdmsrl(MSR_FS_BASE, base);
@@ -135,7 +143,7 @@ static inline unsigned long rdgsbase(void)
 {
     unsigned long base;
 
-    if ( cpu_has_fsgsbase )
+    if ( read_cr4() & X86_CR4_FSGSBASE )
         return __rdgsbase();
 
     rdmsrl(MSR_GS_BASE, base);
@@ -145,7 +153,7 @@ static inline unsigned long rdgsbase(void)
 
 static inline void wrfsbase(unsigned long base)
 {
-    if ( cpu_has_fsgsbase )
+    if ( read_cr4() & X86_CR4_FSGSBASE )
 #ifdef HAVE_GAS_FSGSBASE
         asm volatile ( "wrfsbase %0" :: "r" (base) );
 #else
@@ -157,7 +165,7 @@ static inline void wrfsbase(unsigned long base)
 
 static inline void wrgsbase(unsigned long base)
 {
-    if ( cpu_has_fsgsbase )
+    if ( read_cr4() & X86_CR4_FSGSBASE )
 #ifdef HAVE_GAS_FSGSBASE
         asm volatile ( "wrgsbase %0" :: "r" (base) );
 #else

diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 6a35b89138c52574213c50fb5e44a0d80b60f07c..610b6f3c2641dbd31655ceb24ab944211f27f36c 100644 (file)
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -363,11 +363,31 @@ static inline unsigned long read_cr4(void)
 
 static inline void write_cr4(unsigned long val)
 {
+    struct cpu_info *info = get_cpu_info();
+
     /* No global pages in case of PCIDs enabled! */
     ASSERT(!(val & X86_CR4_PGE) || !(val & X86_CR4_PCIDE));
 
-    get_cpu_info()->cr4 = val;
-    asm volatile ( "mov %0,%%cr4" : : "r" (val) );
+    /*
+     * On hardware supporting FSGSBASE, the value in %cr4 is the kernel's
+     * choice for 64bit PV guests, which impacts whether Xen can use the
+     * instructions.
+     *
+     * The {rd,wr}{fs,gs}base() helpers use info->cr4 to work out whether it
+     * is safe to execute the {RD,WR}{FS,GS}BASE instruction, falling back to
+     * the MSR path if not.  Some users require interrupt safety.
+     *
+     * If FSGSBASE is currently or about to become clear, reflect this in
+     * info->cr4 before updating %cr4, so an interrupt which hits in the
+     * middle won't observe FSGSBASE set in info->cr4 but clear in %cr4.
+     */
+    info->cr4 = val & (info->cr4 | ~X86_CR4_FSGSBASE);
+
+    asm volatile ( "mov %[val], %%cr4"
+                   : "+m" (info->cr4) /* Force ordering without a barrier. */
+                   : [val] "r" (val) );
+
+    info->cr4 = val;
 }
 
 /* Clear and set 'TS' bit respectively */