Re: [PATCH] Re: SSE related security hole

Andrea Arcangeli
2002-04-20T04:20:32Z

On Fri, Apr 19, 2002 at 08:08:15PM -0400, Brian Gerst wrote:
> diff -urN linux-2.5.8/arch/i386/kernel/i387.c linux/arch/i386/kernel/i387.c
> --- linux-2.5.8/arch/i386/kernel/i387.c Thu Mar 7 21:18:32 2002
> +++ linux/arch/i386/kernel/i387.c Fri Apr 19 19:35:14 2002
> @@ -31,13 +31,21 @@
> * value at reset if we support XMM instructions and then
> * remeber the current task has used the FPU.
> */
> -void init_fpu(void)
> +void init_fpu(struct task_struct *tsk)
> {
> - __asm__("fninit");
> - if ( cpu_has_xmm )
> - load_mxcsr(0x1f80);
> -
> - current->used_math = 1;
> + if (cpu_has_fxsr) {
> + memset(&tsk->thread.i387.fxsave, 0, sizeof(struct i387_fxsave_struct));
> + tsk->thread.i387.fxsave.cwd = 0x37f;
> + if (cpu_has_xmm)
> + tsk->thread.i387.fxsave.mxcsr = 0x1f80;
> + } else {
> + memset(&tsk->thread.i387.fsave, 0, sizeof(struct i387_fsave_struct));
> + tsk->thread.i387.fsave.cwd = 0xffff037f;
> + tsk->thread.i387.fsave.swd = 0xffff0000;
> + tsk->thread.i387.fsave.twd = 0xffffffff;
> + tsk->thread.i387.fsave.fos = 0xffff0000;
> + }
> + tsk->used_math = 1;
> }
>
> /*
> diff -urN linux-2.5.8/arch/i386/kernel/traps.c linux/arch/i386/kernel/traps.c
> --- linux-2.5.8/arch/i386/kernel/traps.c Sun Apr 14 23:48:18 2002
> +++ linux/arch/i386/kernel/traps.c Fri Apr 19 18:22:12 2002
> @@ -757,13 +757,12 @@
> */
> asmlinkage void math_state_restore(struct pt_regs regs)
> {
> + struct task_struct *tsk = current;
> clts(); /* Allow maths ops (or we recurse) */
>
> - if (current->used_math) {
> - restore_fpu(current);
> - } else {
> - init_fpu();
> - }
> + if (!tsk->used_math)
> + init_fpu(tsk);
> + restore_fpu(tsk);
> set_thread_flag(TIF_USEDFPU); /* So we fnsave on switch_to() */
> }
>

I don't think it's good enough for merging yet. If you really want to do
the fxrestor, you should at least do the init_fpu only once during
bootup. The fxrestor is probably just overkill, but the memset + the
initializations is completly superflous in a fast path, I'd also use the
proper set_fpu_cwd and friends instead of doing it by hand. Even better
is to merge the:

/* Simulate an empty FPU. */
set_fpu_cwd(child, 0x037f);
set_fpu_swd(child, 0x0000);
set_fpu_twd(child, 0xffff);
set_fpu_mxcsr(child, 0x1f80);

/* Simulate an empty FPU. */
set_fpu_cwd(child, 0x037f);
set_fpu_swd(child, 0x0000);
set_fpu_twd(child, 0xffff);

in ptrace.c in a single function instead of duplicating functionality by
hand.

I still think the xor will be faster, no dcache pollution at all and
less I/O to ram. Future features can require change to the "empty FPU"
state anyways.

Andrea