[v3,2/2] x86/AMD: Fix handling of x87 exception pointers on Fam17h hardware
diff mbox series

Message ID 20190904175708.18853-1-andrew.cooper3@citrix.com
State New, archived
Headers show
Series
  • Untitled series #169711
Related show

Commit Message

Andrew Cooper Sept. 4, 2019, 5:57 p.m. UTC
AMD Pre-Fam17h CPUs "optimise" {F,}X{SAVE,RSTOR} by not saving/restoring
FOP/FIP/FDP if an x87 exception isn't pending.  This causes an information
leak, CVE-2006-1056, and worked around by several OSes, including Xen.  AMD
Fam17h CPUs no longer have this leak, and advertise so in a CPUID bit.

Introduce the RSTR_FP_ERR_PTRS feature, as specified by AMD, and expose to all
guests by default.  While adjusting libxl's cpuid table, add CLZERO which
looks to have been omitted previously.

Also introduce an X86_BUG bit to trigger the (F)XRSTOR workaround, and set it
on AMD hardware where RSTR_FP_ERR_PTRS is not advertised.  Optimise the
conditions for the workaround paths.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v3:
 * Rename to X86_BUG_FPU_PTRS
 * Reinstate, contrary to personal opinion, the fsw/fcw checks.

v2:
 * Use the AMD naming, not that I am convinced this is a sensible name to use.
 * Adjust the i387 codepaths as well as the xstate ones.
 * Add xen-cpuid/libxl data for the CPUID bit.
---
 tools/libxl/libxl_cpuid.c                   |  3 +++
 tools/misc/xen-cpuid.c                      |  1 +
 xen/arch/x86/cpu/amd.c                      |  7 +++++++
 xen/arch/x86/i387.c                         | 16 +++++++---------
 xen/arch/x86/xstate.c                       |  7 +++----
 xen/include/asm-x86/cpufeature.h            |  3 +++
 xen/include/asm-x86/cpufeatures.h           |  2 ++
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 8 files changed, 27 insertions(+), 13 deletions(-)

Comments

Jan Beulich Sept. 5, 2019, 9 a.m. UTC | #1
On 04.09.2019 19:57, Andrew Cooper wrote:
> AMD Pre-Fam17h CPUs "optimise" {F,}X{SAVE,RSTOR} by not saving/restoring
> FOP/FIP/FDP if an x87 exception isn't pending.  This causes an information
> leak, CVE-2006-1056, and worked around by several OSes, including Xen.  AMD
> Fam17h CPUs no longer have this leak, and advertise so in a CPUID bit.
> 
> Introduce the RSTR_FP_ERR_PTRS feature, as specified by AMD, and expose to all
> guests by default.  While adjusting libxl's cpuid table, add CLZERO which
> looks to have been omitted previously.
> 
> Also introduce an X86_BUG bit to trigger the (F)XRSTOR workaround, and set it
> on AMD hardware where RSTR_FP_ERR_PTRS is not advertised.  Optimise the
> conditions for the workaround paths.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
irrespective of a few remarks:

> v3:
>  * Rename to X86_BUG_FPU_PTRS

While I did agree to use this name, I'd still like to point out that
whether or not this is viewed as a bug is a matter of the position
one takes. I'm pretty sure the AMD engineers originally having decided
to avoid saving/restoring these value wouldn't have called this a bug,
but a feature.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -580,6 +580,13 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	}
>  
>  	/*
> +	 * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
> +	 * is pending.  Xen works around this at (F)XRSTOR time.
> +	 */
> +	if ( !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS) )
> +		setup_force_cpu_cap(X86_BUG_FPU_PTRS);

I think in this file you want to omit the blanks immediately inside
the if() parentheses.

> @@ -169,11 +167,11 @@ static inline void fpu_fxsave(struct vcpu *v)
>                         : "=m" (*fpu_ctxt) : "R" (fpu_ctxt) );
>  
>          /*
> -         * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
> -         * is pending.
> +         * Some CPUs don't save/restore FDP/FIP/FOP unless an exception is
> +         * pending.  In this case, the restore side will arrange safe values,
> +         * and there is no point trying to restore FCS/FDS in addition.
>           */
> -        if ( !(fpu_ctxt->fsw & 0x0080) &&
> -             boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +        if ( cpu_bug_fpu_ptrs && !(fpu_ctxt->fsw & 0x0080) )
>              return;

Could I talk you into s/trying to restore/trying to collect/ for the
comment? The consumer of the collected data could in principle be
other than the corresponding restore code, e.g. the insn emulator.
(hvmemul_put_fpu() has an example of the opposite direction, i.e.
producing data for the restore logic to consume.)

Jan
Andrew Cooper Sept. 5, 2019, 11:36 a.m. UTC | #2
On 05/09/2019 10:00, Jan Beulich wrote:
> On 04.09.2019 19:57, Andrew Cooper wrote:
>> AMD Pre-Fam17h CPUs "optimise" {F,}X{SAVE,RSTOR} by not saving/restoring
>> FOP/FIP/FDP if an x87 exception isn't pending.  This causes an information
>> leak, CVE-2006-1056, and worked around by several OSes, including Xen.  AMD
>> Fam17h CPUs no longer have this leak, and advertise so in a CPUID bit.
>>
>> Introduce the RSTR_FP_ERR_PTRS feature, as specified by AMD, and expose to all
>> guests by default.  While adjusting libxl's cpuid table, add CLZERO which
>> looks to have been omitted previously.
>>
>> Also introduce an X86_BUG bit to trigger the (F)XRSTOR workaround, and set it
>> on AMD hardware where RSTR_FP_ERR_PTRS is not advertised.  Optimise the
>> conditions for the workaround paths.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> irrespective of a few remarks:
>
>> v3:
>>  * Rename to X86_BUG_FPU_PTRS
> While I did agree to use this name, I'd still like to point out that
> whether or not this is viewed as a bug is a matter of the position
> one takes. I'm pretty sure the AMD engineers originally having decided
> to avoid saving/restoring these value wouldn't have called this a bug,
> but a feature.

I accept that different people might have different opinions on the
matter, but at the point that we and every other large software vendor
has issued a security fix because of it, it can't credibly be called a
feature, irrespective of the original intention.

The change of behaviour on Fam17h is either tacit agreement with this
point, or at least a begrudging acceptance that the only way the
workaround is going to go away is by changing the behaviour of the CPU.

>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -580,6 +580,13 @@ static void init_amd(struct cpuinfo_x86 *c)
>>  	}
>>  
>>  	/*
>> +	 * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
>> +	 * is pending.  Xen works around this at (F)XRSTOR time.
>> +	 */
>> +	if ( !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS) )
>> +		setup_force_cpu_cap(X86_BUG_FPU_PTRS);
> I think in this file you want to omit the blanks immediately inside
> the if() parentheses.

Oops yes.

>
>> @@ -169,11 +167,11 @@ static inline void fpu_fxsave(struct vcpu *v)
>>                         : "=m" (*fpu_ctxt) : "R" (fpu_ctxt) );
>>  
>>          /*
>> -         * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
>> -         * is pending.
>> +         * Some CPUs don't save/restore FDP/FIP/FOP unless an exception is
>> +         * pending.  In this case, the restore side will arrange safe values,
>> +         * and there is no point trying to restore FCS/FDS in addition.
>>           */
>> -        if ( !(fpu_ctxt->fsw & 0x0080) &&
>> -             boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>> +        if ( cpu_bug_fpu_ptrs && !(fpu_ctxt->fsw & 0x0080) )
>>              return;
> Could I talk you into s/trying to restore/trying to collect/ for the
> comment? The consumer of the collected data could in principle be
> other than the corresponding restore code, e.g. the insn emulator.
> (hvmemul_put_fpu() has an example of the opposite direction, i.e.
> producing data for the restore logic to consume.)

Ok.

~Andrew

Patch
diff mbox series

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index f1c6ce2076..953a3bbd8c 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -257,8 +257,11 @@  int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
 
         {"invtsc",       0x80000007, NA, CPUID_REG_EDX,  8,  1},
 
+        {"clzero",       0x80000008, NA, CPUID_REG_EBX,  0,  1},
+        {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
         {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
         {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
+
         {"nc",           0x80000008, NA, CPUID_REG_ECX,  0,  8},
         {"apicidsize",   0x80000008, NA, CPUID_REG_ECX, 12,  4},
 
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index be6a8d27a5..f51facffb6 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -145,6 +145,7 @@  static const char *const str_e7d[32] =
 static const char *const str_e8b[32] =
 {
     [ 0] = "clzero",
+    [ 2] = "rstr-fp-err-ptrs",
 
     /* [ 8] */            [ 9] = "wbnoinvd",
 
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index a2f83c79a5..dc9ed55ba6 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -580,6 +580,13 @@  static void init_amd(struct cpuinfo_x86 *c)
 	}
 
 	/*
+	 * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
+	 * is pending.  Xen works around this at (F)XRSTOR time.
+	 */
+	if ( !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS) )
+		setup_force_cpu_cap(X86_BUG_FPU_PTRS);
+
+	/*
 	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
 	 * certainly isn't virtualised (and Xen at least will leak the real
 	 * value in but silently discard writes), as well as being per-core
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 88178485cb..e4f0965eed 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -43,20 +43,18 @@  static inline void fpu_fxrstor(struct vcpu *v)
     const typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
 
     /*
-     * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+     * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
      * is pending. Clear the x87 state here by setting it to fixed
      * values. The hypervisor data segment can be sometimes 0 and
      * sometimes new user value. Both should be ok. Use the FPU saved
      * data block as a safe address because it should be in L1.
      */
-    if ( !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) &&
-         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
-    {
+    if ( cpu_bug_fpu_ptrs &&
+         !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) )
         asm volatile ( "fnclex\n\t"
                        "ffree %%st(7)\n\t" /* clear stack tag */
                        "fildl %0"          /* load to clear state */
                        : : "m" (*fpu_ctxt) );
-    }
 
     /*
      * FXRSTOR can fault if passed a corrupted data block. We handle this
@@ -169,11 +167,11 @@  static inline void fpu_fxsave(struct vcpu *v)
                        : "=m" (*fpu_ctxt) : "R" (fpu_ctxt) );
 
         /*
-         * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
-         * is pending.
+         * Some CPUs don't save/restore FDP/FIP/FOP unless an exception is
+         * pending.  In this case, the restore side will arrange safe values,
+         * and there is no point trying to restore FCS/FDS in addition.
          */
-        if ( !(fpu_ctxt->fsw & 0x0080) &&
-             boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+        if ( cpu_bug_fpu_ptrs && !(fpu_ctxt->fsw & 0x0080) )
             return;
 
         /*
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 3293ef834f..10016a05d0 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -369,15 +369,14 @@  void xrstor(struct vcpu *v, uint64_t mask)
     unsigned int faults, prev_faults;
 
     /*
-     * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+     * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
      * is pending. Clear the x87 state here by setting it to fixed
      * values. The hypervisor data segment can be sometimes 0 and
      * sometimes new user value. Both should be ok. Use the FPU saved
      * data block as a safe address because it should be in L1.
      */
-    if ( (mask & ptr->xsave_hdr.xstate_bv & X86_XCR0_FP) &&
-         !(ptr->fpu_sse.fsw & ~ptr->fpu_sse.fcw & 0x003f) &&
-         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+    if ( cpu_bug_fpu_ptrs &&
+         !(ptr->fpu_sse.fsw & ~ptr->fpu_sse.fcw & 0x003f) )
         asm volatile ( "fnclex\n\t"        /* clear exceptions */
                        "ffree %%st(7)\n\t" /* clear stack tag */
                        "fildl %0"          /* load to clear state */
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 7e1ff17ad4..00d22caac7 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -138,6 +138,9 @@ 
 
 #define cpu_has_msr_tsc_aux     (cpu_has_rdtscp || cpu_has_rdpid)
 
+/* Bugs. */
+#define cpu_bug_fpu_ptrs        boot_cpu_has(X86_BUG_FPU_PTRS)
+
 enum _cache_type {
     CACHE_TYPE_NULL = 0,
     CACHE_TYPE_DATA = 1,
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index ab3650f73b..91eccf5161 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -43,5 +43,7 @@  XEN_CPUFEATURE(SC_VERW_IDLE,      X86_SYNTH(25)) /* VERW used by Xen for idle */
 #define X86_NR_BUG 1
 #define X86_BUG(x) ((FSCAPINTS + X86_NR_SYNTH) * 32 + (x))
 
+#define X86_BUG_FPU_PTRS          X86_BUG( 0) /* (F)X{SAVE,RSTOR} doesn't save/restore FOP/FIP/FDP. */
+
 /* Total number of capability words, inc synth and bug words. */
 #define NCAPINTS (FSCAPINTS + X86_NR_SYNTH + X86_NR_BUG) /* N 32-bit words worth of info */
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index f2ec470179..48d8d1f4e2 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -244,6 +244,7 @@  XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF Read Only interface */
 
 /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
 XEN_CPUFEATURE(CLZERO,        8*32+ 0) /*A  CLZERO instruction */
+XEN_CPUFEATURE(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A  (F)X{SAVE,RSTOR} always saves/restores FPU Error pointers */
 XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*   WBNOINVD instruction */
 XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by AMD) */