Here's a revised diff to correct the handling on amd64 and i386 of the 
MXCSR register in frames that could be altered by users, so that a user 
can't trick the kernel into faulting by trying to load an invalid MXCSR 
value during the return to userspace.

Originally worked out by joshe@; this corrects the timing of the call to 
fpuinit() for the primary CPU on amd64 so that the saved mask value is 
initialized correctly.  It also uniformly uses 
__attribute__((aligned(16))) do get a buffer of the correct alignment.

I've been using this on amd64 for a while and have confirmed that a test 
program which previous crashed the box now continues just fine.  I'm away 
from my i386 test box so we could use confirmation from someone running an 
i386 that shows FXSR in the cpu0 flags list.

ok?


Philip Guenther

Index: sys/arch/amd64/amd64/cpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
retrieving revision 1.40
diff -u -p -r1.40 cpu.c
--- sys/arch/amd64/amd64/cpu.c	27 Nov 2010 13:03:04 -0000	1.40
+++ sys/arch/amd64/amd64/cpu.c	25 Dec 2010 19:38:48 -0000
@@ -365,6 +365,8 @@ cpu_attach(struct device *parent, struct
 void
 cpu_init(struct cpu_info *ci)
 {
+	fpuinit(ci);
+
 	/* configure the CPU if needed */
 	if (ci->cpu_setup != NULL)
 		(*ci->cpu_setup)(ci);
@@ -509,7 +511,6 @@ cpu_hatch(void *v)
 	cpu_init_idt();
 	lapic_set_lvt();
 	gdt_init_cpu(ci);
-	fpuinit(ci);
 
 	lldt(0);
 
Index: sys/arch/amd64/amd64/fpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
retrieving revision 1.21
diff -u -p -r1.21 fpu.c
--- sys/arch/amd64/amd64/fpu.c	29 Sep 2010 15:11:31 -0000	1.21
+++ sys/arch/amd64/amd64/fpu.c	25 Dec 2010 19:38:48 -0000
@@ -95,6 +95,8 @@
 void fpudna(struct cpu_info *);
 static int x86fpflags_to_siginfo(u_int32_t);
 
+uint32_t	fpu_mxcsr_mask;
+
 /*
  * Init the FPU.
  */
@@ -103,6 +105,16 @@ fpuinit(struct ...
[ message continues ]

Appears to work (i.e. boot) on both my amd64 and i386 boxes. The
latter has FXSR in the cpu0 flags. Just in case I am hallucinating,
the i386 dmesg is below.

.... Ken

OpenBSD 4.8-current (GENERIC.MP) #0: Sun Dec 26 08:56:56 EST 2010
    root@acer.westerback.ca:/usr/src/sys/arch/i386/compile/GENERIC.MP
cpu0: Pentium(R) Dual-Core CPU E5400 @ 2.70GHz ("GenuineIntel" 686-class) 2.71 GHz
cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,SBF,SSE3,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,XSAVE
real mem  = 2683523072 (2559MB)
avail mem = 2629521408 (2507MB)
mainbus0 at root
bios0 at mainbus0: AT/286+ BIOS, date 10/14/09, SMBIOS rev. 2.6 @ 0x9f400 (38 entries)
bios0: vendor American Megatrends Inc. version "P01-A4" date 10/14/2009
bios0: Acer Aspire X1800
acpi0 at bios0: rev 0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP APIC MCFG SLIC WDRT OEMB HPET NVHD AWMI SSDT
acpi0: wakeup devices PS2K(S4) PS2M(S4) NSMB(S4) USB0(S3) USB2(S3) NMAC(S5) P0P1(S4) HDAC(S4) BR10(S4) BR11(S4) BR12(S4) PWRB(S4)
acpitimer0 at acpi0: 3579545 Hz, 32 bits
acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: apic clock running at 200MHz
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Pentium(R) Dual-Core CPU E5400 @ 2.70GHz ("GenuineIntel" 686-class) 2.71 GHz
cpu1: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,SBF,SSE3,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,XSAVE
ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 11, 24 pins
acpihpet0 at acpi0: 25000000 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (P0P1)
acpiprt2 at acpi0: bus 2 (BR10)
acpiprt3 at acpi0: bus 3 (BR11)
acpiprt4 at acpi0: bus 4 (BR12)
acpicpu0 at acpi0: PSS
acpicpu1 at acpi0: PSS
acpitz0 at acpi0: critical temperature 125 degC
acpibtn0 at acpi0: PWRB
bios0: ROM list: 0xc0000/0xde00
cpu0: Enhanced SpeedStep 2701 ...
[ message continues ]

Hmm, I'm not too happy about moving fpuinit() out of cpu_hatch() on
amd64, while leaving npxinit() in there for i386.


This function will be run again upon resume.  Now overwriting
fpu_mxcsr_mask shouldn't hurt, but perhaps replacing:

	if (CPU_IS_PRIMARY(ci)) {

with

	if (fpu_mxcsr_mask == 0) {


Oh, this actually points out an interesting problem.  The MXCSR
register isn't initialized from data passed to us by userland.
However, if a user calls ptrace(PT_SETFPREGS, ...) on a process that
didn't use the FPU yet, it isn't quite clear what we initialize the
XMM state to.  While your approach will make sure we won't cause a
segfault, we might still be leaking stuff from the kernel to userland.
I think the diff below is a better way to address the issue.

Index: process_machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/process_machdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 process_machdep.c
--- process_machdep.c	29 Sep 2010 15:11:31 -0000	1.25
+++ process_machdep.c	26 Dec 2010 15:23:33 -0000
@@ -299,8 +299,15 @@ process_write_fpregs(struct proc *p, str
 #if NNPX > 0
 		npxsave_proc(p, 0);
 #endif
-	} else
+	} else {
+		/*
+		 * Make sure MXCSR and the XMM registers are
+		 * initialized to sane defaults.
+		 */
+		if (i386_use_fxsave)
+			process_fninit_xmm(&frame->sv_xmm);
 		p->p_md.md_flags |= MDP_USEDFPU;
+	}
 
 	if (i386_use_fxsave) {
 		struct save87 s87;

[ message continues ]

On Sun, Dec 26, 2010 at 7:24 AM, Mark Kettenis <mark.kettenis@xs4all.nl>

Well, on i386, npxinit() is called for the primary cpu when the npx is
attached, which is way after the secondary cpus are attached.  Can we
assume it to be safe to call npxinit() before the npx device is
attached?  It wouldn't surprise me to hear that it only works when npx
errors are reported using exceptions and not ISA interrupts...which
would explain why the early calls are fine on multi-CPU systems,
'cause they all use exceptions.

So no, I don't think it's safe to move up the npxinit() call into
cpu_init(), unless you're suggesting that we delay initializing
secondary cpus until after the isa bus is probed (barf).  The
consistent alternative would be to leave the fpuinit() call in
cpu_hatch() and call it for the primary cpu near the end of

No.  The primary cpu check is necessary, at least on i386, because
secondary cpus call npxinit() before the primary cpu *and* before
setting CR4_OSFXSR.

(Also, you're not planning on hot-upgrading your FPU by replacing the

Yes, I agree.  IMO, that can be committed separately (ok guenther@)
from this mxcsr_mask diff.

What this really points out, however, is that the masking added by my
diff was in the wrong function: it should have been in
process_write_xmmregs(), duh.  I'll fix that in the next rev.

Once we agree where to npxinit()/fpuinit(), I'll post an updated diff.


Philip Guenther

[ message continues ]

Looks like you're fooled by another not-so-subtle difference between
i386 and amd64.  On amd64 the CPUs are spun up when they attach.  On
i386 that doesn't happen until we call cpu_boot_secondary_processors()
in main(), which is long after npx(4) attaches and npxinit() gets




Right.

[ message continues ]

On Mon, 27 Dec 2010, Mark Kettenis wrote:

Ah, I missed the CPUF_GO bit.  Gotta love the simplicity of the MP boot 

Okay, check out the revised diff below.  I've tested the updated amd64 
part; I would appreciate a confirmation from an i386 w/X11 user for that 
part.


Philip


Index: amd64/amd64/fpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
retrieving revision 1.21
diff -u -p -r1.21 fpu.c
--- amd64/amd64/fpu.c	29 Sep 2010 15:11:31 -0000	1.21
+++ amd64/amd64/fpu.c	27 Dec 2010 20:05:56 -0000
@@ -95,6 +95,8 @@
 void fpudna(struct cpu_info *);
 static int x86fpflags_to_siginfo(u_int32_t);
 
+uint32_t	fpu_mxcsr_mask;
+
 /*
  * Init the FPU.
  */
@@ -103,6 +105,16 @@ fpuinit(struct cpu_info *ci)
 {
 	lcr0(rcr0() & ~(CR0_EM|CR0_TS));
 	fninit();
+	if (fpu_mxcsr_mask == 0) {
+		struct fxsave64 fx __attribute__((aligned(16)));
+
+		bzero(&fx, sizeof(fx));
+		fxsave(&fx);
+		if (fx.fx_mxcsr_mask)
+			fpu_mxcsr_mask = fx.fx_mxcsr_mask;
+		else
+			fpu_mxcsr_mask = __INITIAL_MXCSR_MASK__;
+	}
 	lcr0(rcr0() | (CR0_TS));
 }
 
Index: amd64/amd64/machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.129
diff -u -p -r1.129 machdep.c
--- amd64/amd64/machdep.c	22 Nov 2010 21:07:16 -0000	1.129
+++ amd64/amd64/machdep.c	27 Dec 2010 20:05:56 -0000
@@ -658,9 +658,11 @@ sys_sigreturn(struct proc *p, void *v, r
 		fpusave_proc(p, 0);
 
 	if (ksc.sc_fpstate) {
-		if ((error = copyin(ksc.sc_fpstate,
-		    &p->p_addr->u_pcb.pcb_savefpu.fp_fxsave, sizeof (struct fxsave64))))
+		struct fxsave64 *fx = &p->p_addr->u_pcb.pcb_savefpu.fp_fxsave;
+
+		if ((error = copyin(ksc.sc_fpstate, fx, sizeof(*fx))))
 			return (error);
+		fx->fx_mxcsr &= fpu_mxcsr_mask;
 		p->p_md.md_flags |= MDP_USEDFPU;
 	}
 
@@ -1506,6 +1508,7 @@ init_x86_64(paddr_t first_avail)
 	cpu_init_idt();
 
 ...
[ message continues ]

Works for me, X runs and test program no longer drops us into ddb:

OpenBSD 4.8-current (GENERIC) #43: Mon Dec 27 13:08:01 PST 2010
    joshe@flint.joshe.fake:/usr/src/sys/arch/i386/compile/GENERIC
cpu0: Intel(R) Celeron(R) M processor 1.40GHz ("GenuineIntel" 686-class) 1.40 GHz
cpu0: FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,SEP,MTRR,PGE,MCA,CMOV,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,TM,SBF

It couldn't hurt to also test this on a machine where the fxsave
instruction writes an empty mask. This should be any machine with SSE
is in the cpu flags list and without SSE2.


[ message continues ]