Message-Id: <20170908164955.41784-1-ebiggers3@gmail.com>
Date:   Fri,  8 Sep 2017 09:49:55 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, Eric Biggers <ebiggers3@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Ingo Molnar <mingo@...nel.org>, Kevin Hao <haokexin@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Wanpeng Li <wanpeng.li@...mail.com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>, stable@...r.kernel.org
Subject: [PATCH] x86/fpu: don't let PTRACE_SETREGSET set xcomp_bv

From: Eric Biggers <ebiggers@...gle.com>

On x86, userspace can use ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE,
&iov) to set a task's extended state registers.  Registers can be set to
any value, but the kernel assumes that the xregs_state itself remains
valid in the sense that the CPU can restore it.

However, in the case where the kernel is using the non-compacted
extended state format (which it does whenever the XSAVES instruction is
unavailable), userspace could also set the xcomp_bv field of the
xstate_header to any value.  But all bits in that field are reserved in
the non-compacted case, so when switching to a task with nonzero
xcomp_bv, the XRSTOR instruction failed with a #GP fault.  This caused
the WARN_ON_FPU(err) in copy_kernel_to_xregs() to be hit and, since the
error is otherwise ignored, could also leak registers from the task
previously executing on the CPU.

Fix the bug by ignoring the user-specified value of xcomp_bv in the
non-compacted case and instead always setting it to 0.

Note: we used to clear all xstate registers if XRSTOR failed, but this
was removed by commit 9ccc27a5d297 ("x86/fpu: Remove error return values
from copy_kernel_to_*regs() functions").  It perhaps should be added
back, to prevent this class of bug from causing an information leak.

This bug was found by syzkaller, which hit the above-mentioned
WARN_ON_FPU():

    WARNING: CPU: 1 PID: 0 at ./arch/x86/include/asm/fpu/internal.h:373 __switch_to+0x5b5/0x5d0
    CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.13.0 #453
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    task: ffff9ba2bc8e42c0 task.stack: ffffa78cc036c000
    RIP: 0010:__switch_to+0x5b5/0x5d0
    RSP: 0000:ffffa78cc08bbb88 EFLAGS: 00010082
    RAX: 00000000fffffffe RBX: ffff9ba2b8bf2180 RCX: 00000000c0000100
    RDX: 00000000ffffffff RSI: 000000005cb10700 RDI: ffff9ba2b8bf36c0
    RBP: ffffa78cc08bbbd0 R08: 00000000929fdf46 R09: 0000000000000001
    R10: 0000000000000000 R11: 0000000000000000 R12: ffff9ba2bc8e42c0
    R13: 0000000000000000 R14: ffff9ba2b8bf3680 R15: ffff9ba2bf5d7b40
    FS:  00007f7e5cb10700(0000) GS:ffff9ba2bf400000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 00000000004005cc CR3: 0000000079fd5000 CR4: 00000000001406e0
    Call Trace:
    Code: 84 00 00 00 00 00 e9 11 fd ff ff 0f ff 66 0f 1f 84 00 00 00 00 00 e9 e7 fa ff ff 0f ff 66 0f 1f 84 00 00 00 00 00 e9 c2 fa ff ff <0f> ff 66 0f 1f 84 00 00 00 00 00 e9 d4 fc ff ff 66 66 2e 0f 1f

The following C program reproduces the bug quite reliably.  The expected
behavior is that the program spin forever with no output.  However, on a
buggy kernel running on a processor with the "xsave" feature but without
the "xsaves" feature (e.g. Sandy Bridge through Broadwell for Intel),
within a second or two the program reports that the xmm registers were
corrupted, i.e. were not restored correctly.  With
CONFIG_X86_DEBUG_FPU=y it also hits the above kernel warning.

    #define _GNU_SOURCE
    #include <stdbool.h>
    #include <inttypes.h>
    #include <linux/elf.h>
    #include <stdio.h>
    #include <sys/ptrace.h>
    #include <sys/uio.h>
    #include <sys/wait.h>
    #include <unistd.h>

    int main(void)
    {
        int pid = fork();
        uint64_t xstate[512];
        struct iovec iov = { .iov_base = xstate, .iov_len = sizeof(xstate) };

        if (pid == 0) {
            bool tracee = true;
            for (int i = 0; i < sysconf(_SC_NPROCESSORS_ONLN) && tracee; i++)
                tracee = (fork() != 0);
            uint32_t xmm0[4] = { [0 ... 3] = tracee ? 0x12345678 : 0xDEADBEEF };
            asm volatile("   movdqu %0, %%xmm0\n"
                         "   mov %0, %%rbx\n"
                         "1: movdqu %%xmm0, %0\n"
                         "   mov %0, %%rax\n"
                         "   cmp %%rax, %%rbx\n"
                         "   je 1b\n"
                         : "+m" (xmm0) : : "rax", "rbx", "xmm0");
            printf("BUG: xmm registers corrupted!  tracee=%d, xmm0=%08X%08X%08X%08X\n",
                   tracee, xmm0[0], xmm0[1], xmm0[2], xmm0[3]);
        } else {
            usleep(100000);
            ptrace(PTRACE_ATTACH, pid, 0, 0);
            wait(NULL);
            ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov);
            xstate[65] = -1;
	    ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov);
	    ptrace(PTRACE_CONT, pid, 0, 0);
	    wait(NULL);
	}
	return 1;
    }

Fixes: 0b29643a5843 ("x86/xsaves: Change compacted format xsave area header")
Cc: Andy Lutomirski <luto@...nel.org>
Cc: Dave Hansen <dave.hansen@...ux.intel.com>
Cc: Dmitry Vyukov <dvyukov@...gle.com>
Cc: Fenghua Yu <fenghua.yu@...el.com>
Cc: Ingo Molnar <mingo@...nel.org>
Cc: Kevin Hao <haokexin@...il.com>
Cc: Oleg Nesterov <oleg@...hat.com>
Cc: Wanpeng Li <wanpeng.li@...mail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@...el.com>
Cc: <stable@...r.kernel.org>    [v3.17+]
Signed-off-by: Eric Biggers <ebiggers@...gle.com>
---
 arch/x86/kernel/fpu/regset.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index b188b16841e3..718b791bc037 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -131,11 +131,15 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	fpu__activate_fpstate_write(fpu);
 
-	if (boot_cpu_has(X86_FEATURE_XSAVES))
+	if (boot_cpu_has(X86_FEATURE_XSAVES)) {
 		ret = copyin_to_xsaves(kbuf, ubuf, xsave);
-	else
+	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 
+		/* xcomp_bv must be zero when using uncompacted format */
+		xsave->header.xcomp_bv = 0;
+	}
+
 	/*
 	 * In case of failure, mark all states as init:
 	 */
-- 
2.14.1.581.gf28d330327-goog

Powered by blists - more mailing lists