[PATCH] fxsave/fxrstor adjustments

[PATCH] fxsave/fxrstor adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

This patch addresses CVE-2006-1056 (information leak from fxsave/fxrstor on AMD CPUs) and also adjusts 64-bit handling
so that full 64-bit RIP/RDP values get saved/restored. More fine-grained handling may be needed if 32-bit processes are
expected to properly see their selectors (native Linux doesn't currently do that either, but there is a patch to adjust
it there).

Signed-off-by: Jan Beulich <jbeulich@novell.com>


[-- Attachment #2: xen-x86-fxsr.patch --]
[-- Type: text/plain, Size: 4036 bytes --]

--- 2006-04-21/xen/arch/x86/i387.c.0	2006-04-06 17:50:26.000000000 +0200
+++ 2006-04-21/xen/arch/x86/i387.c	2006-04-24 16:23:21.000000000 +0200
@@ -31,14 +31,55 @@ void save_init_fpu(struct vcpu *v)
     if ( cr0 & X86_CR0_TS )
         clts();
 
+#ifdef __i386__
     if ( cpu_has_fxsr )
         __asm__ __volatile__ (
-            "fxsave %0 ; fnclex"
+            "fxsave %0"
             : "=m" (v->arch.guest_context.fpu_ctxt) );
     else
         __asm__ __volatile__ (
             "fnsave %0 ; fwait"
             : "=m" (v->arch.guest_context.fpu_ctxt) );
+#elif 0
+    /* Using "fxsaveq %0" would be the ideal choice, but is only supported
+       starting with gas 2.16. */
+    __asm__ __volatile__ (
+        "fxsaveq %0"
+        : "=m" (v->arch.guest_context.fpu_ctxt) );
+#elif 0
+    /* Using, as a workaround, the properly prefixed form below isn't
+       accepted by any binutils version so far released, complaining that
+       the same type of prefix is used twice if an extended register is
+       needed for addressing (fix submitted to mainline 2005-11-21). */
+    __asm__ __volatile__ (
+        "rex64/fxsave %0"
+        : "=m" (v->arch.guest_context.fpu_ctxt) );
+#else
+    /* This, however, we can work around by forcing the compiler to select
+       an addressing mode that doesn't require extended registers. */
+    __asm__ __volatile__ (
+        "rex64/fxsave %P2(%1)"
+        : "=m" (v->arch.guest_context.fpu_ctxt)
+        : "cdaSDb" (v),
+          "i" (offsetof(__typeof__(*v), arch.guest_context.fpu_ctxt)) );
+#endif
+    if ( cpu_has_fxsr ) {
+        if ( unlikely(v->arch.guest_context.fpu_ctxt.x[2] & 0x80) )
+            __asm__ __volatile__ ("fnclex");
+        /* AMD 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 boot CPU's capability field as safe address because it
+           (being read in the conditional above) should be already in L1. */
+        if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) {
+            __asm__ __volatile__ (
+                "emms\n\t"  /* clear stack tags */
+	     	 "fildl %0"  /* load to clear state */
+	     	 :
+	     	 : "m" (boot_cpu_data.x86_capability[X86_FEATURE_FXSR / 32]));
+        }
+    }
 
     clear_bit(_VCPUF_fpu_dirtied, &v->vcpu_flags);
     write_cr0(cr0|X86_CR0_TS);
@@ -51,6 +92,7 @@ void restore_fpu(struct vcpu *v)
      * possibility, which may occur if the block was passed to us by control
      * tools, by silently clearing the block.
      */
+#ifdef __i386__
     if ( cpu_has_fxsr )
         __asm__ __volatile__ (
             "1: fxrstor %0            \n"
@@ -78,6 +120,33 @@ void restore_fpu(struct vcpu *v)
         __asm__ __volatile__ (
             "frstor %0"
             : : "m" (v->arch.guest_context.fpu_ctxt) );
+#else
+    /* See save_init_fpu() for why the operands/constraints are this way. */
+    __asm__ __volatile__ (
+        "1: rex64/fxrstor %P3(%2) \n"
+        ".section .fixup,\"ax\"   \n"
+        "2: push %%"__OP"ax       \n"
+        "   push %%"__OP"cx       \n"
+        "   push %%"__OP"di       \n"
+        "   lea  %0,%%"__OP"di    \n"
+        "   mov  %1,%%ecx         \n"
+        "   xor  %%eax,%%eax      \n"
+        "   rep ; stosl           \n"
+        "   pop  %%"__OP"di       \n"
+        "   pop  %%"__OP"cx       \n"
+        "   pop  %%"__OP"ax       \n"
+        "   jmp  1b               \n"
+        ".previous                \n"
+        ".section __ex_table,\"a\"\n"
+        "   "__FIXUP_ALIGN"       \n"
+        "   "__FIXUP_WORD" 1b,2b  \n"
+        ".previous                \n"
+        : 
+        : "m" (v->arch.guest_context.fpu_ctxt),
+          "i" (sizeof(v->arch.guest_context.fpu_ctxt)/4),
+          "cdaSDb" (v),
+          "i" (offsetof(__typeof__(*v), arch.guest_context.fpu_ctxt)) );
+#endif
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] fxsave/fxrstor adjustments

[Keir Fraser]

On 24 Apr 2006, at 16:31, Jan Beulich wrote:

> This patch addresses CVE-2006-1056 (information leak from 
> fxsave/fxrstor on AMD CPUs) and also adjusts 64-bit handling
> so that full 64-bit RIP/RDP values get saved/restored. More 
> fine-grained handling may be needed if 32-bit processes are
> expected to properly see their selectors (native Linux doesn't 
> currently do that either, but there is a patch to adjust
> it there).

Why does this patch (and the one in Linux 2.6.16.9) use 'emms' in the 
fxsave path rather than 'ffree st(7)' which is what AMD recommends in 
their published advisory? Is the former faster?

  -- Keir

Re: [PATCH] fxsave/fxrstor adjustments

[Andi Kleen]

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:

> On 24 Apr 2006, at 16:31, Jan Beulich wrote:
> 
> > This patch addresses CVE-2006-1056 (information leak from
> > fxsave/fxrstor on AMD CPUs) and also adjusts 64-bit handling
> > so that full 64-bit RIP/RDP values get saved/restored. More
> > fine-grained handling may be needed if 32-bit processes are
> > expected to properly see their selectors (native Linux doesn't
> > currently do that either, but there is a patch to adjust
> > it there).
> 
> Why does this patch (and the one in Linux 2.6.16.9) use 'emms' in the
> fxsave path rather than 'ffree st(7)' which is what AMD recommends in
> their published advisory? Is the former faster?

On K7/K8 emms and ffree st(7) are the same performance. On P4 ffree is
much faster. The Linux 2.6 patch uses emms because it patches the code
in only on K7/K8. For the Xen patch that's ok too although it checks,
not patches. In Linux 2.4 where the code is executed unconditionally
ffree is used.

An earlier version of the AMD workaround used emms always until
the P4 emms performance issue was discovered.

-Andi

Re: [PATCH] fxsave/fxrstor adjustments

[Keir Fraser]

On 25 Apr 2006, at 01:56, Andi Kleen wrote:

> On K7/K8 emms and ffree st(7) are the same performance. On P4 ffree is
> much faster. The Linux 2.6 patch uses emms because it patches the code
> in only on K7/K8. For the Xen patch that's ok too although it checks,
> not patches. In Linux 2.4 where the code is executed unconditionally
> ffree is used.
>
> An earlier version of the AMD workaround used emms always until
> the P4 emms performance issue was discovered.

Thanks Andi.

  -- Keir

Re: [PATCH] fxsave/fxrstor adjustments

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 24.04.06 18:14 >>>
>
>On 24 Apr 2006, at 16:31, Jan Beulich wrote:
>
>> This patch addresses CVE-2006-1056 (information leak from 
>> fxsave/fxrstor on AMD CPUs) and also adjusts 64-bit handling
>> so that full 64-bit RIP/RDP values get saved/restored. More 
>> fine-grained handling may be needed if 32-bit processes are
>> expected to properly see their selectors (native Linux doesn't 
>> currently do that either, but there is a patch to adjust
>> it there).
>
>Why does this patch (and the one in Linux 2.6.16.9) use 'emms' in the 
>fxsave path rather than 'ffree st(7)' which is what AMD recommends in 
>their published advisory? Is the former faster?

I just cloned what Andi did for 32- and 64-bit Linux. Andi?

Jan