[PATCH] x86/fpu: don't let PTRACE_SETREGSET set xcomp_bv

[PATCH] x86/fpu: don't let PTRACE_SETREGSET set xcomp_bv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.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@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: <stable@vger.kernel.org>    [v3.17+]
Signed-off-by: Eric Biggers <ebiggers@google.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

Re: [PATCH] x86/fpu: don't let PTRACE_SETREGSET set xcomp_bv

[Dave Hansen]

On 09/08/2017 09:49 AM, Eric Biggers wrote:
> 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;
> +	}

Thanks for finding this!

I wonder if just writing over the user arguments is the right thing
here.  It's quite conceivable that userspace has a *real* compacted
(XSAVEC-generated) buffer.  Doing this could still end up corrupting data.

I think we probably (re?) define NT_X86_XSTATE as being for uncompacted
state only.  Then, return an error back to userspace to tell them they
tried to do something we can't support.

We might even want to check all the reserved bits in the uncompacted
XSAVE header and enforce that they come in as zero.