[PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This series fixes the bug found by syzkaller where the ptrace syscall
can be used to set invalid bits in a task's FPU state.  I also found
that an equivalent bug was reachable using the sigreturn syscall, so the
first patch fixes the bug in both cases.

The other two patches start validating the other parts of the
xstate_header and make it so that invalid FPU states can no longer be
abused to leak the FPU registers of other processes.

Changes since v2:
    - Use an exception handler to handle invalid FPU states
      (suggested by Andy Lutomirski)
    - Check the size of xstate_header.reserved at build time
      (suggested by Dave Hansen)

Eric Biggers (3):
  x86/fpu: don't let userspace set bogus xcomp_bv
  x86/fpu: tighten validation of user-supplied xstate_header
  x86/fpu: reinitialize FPU registers if restoring FPU state fails

 arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
 arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
 arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
 arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
 arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
 arch/x86/mm/extable.c               | 24 +++++++++++++++++
 6 files changed, 94 insertions(+), 68 deletions(-)

-- 
2.14.1.821.g8fa685d3b7-goog

[kernel-hardening] [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This series fixes the bug found by syzkaller where the ptrace syscall
can be used to set invalid bits in a task's FPU state.  I also found
that an equivalent bug was reachable using the sigreturn syscall, so the
first patch fixes the bug in both cases.

The other two patches start validating the other parts of the
xstate_header and make it so that invalid FPU states can no longer be
abused to leak the FPU registers of other processes.

Changes since v2:
    - Use an exception handler to handle invalid FPU states
      (suggested by Andy Lutomirski)
    - Check the size of xstate_header.reserved at build time
      (suggested by Dave Hansen)

Eric Biggers (3):
  x86/fpu: don't let userspace set bogus xcomp_bv
  x86/fpu: tighten validation of user-supplied xstate_header
  x86/fpu: reinitialize FPU registers if restoring FPU state fails

 arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
 arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
 arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
 arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
 arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
 arch/x86/mm/extable.c               | 24 +++++++++++++++++
 6 files changed, 94 insertions(+), 68 deletions(-)

-- 
2.14.1.821.g8fa685d3b7-goog

[PATCH v3 1/3] x86/fpu: don't let userspace set bogus xcomp_bv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

On x86, userspace can use the ptrace() or rt_sigreturn() system calls to
set a task's extended state (xstate) or "FPU" registers.  ptrace() can
set them for another task using the PTRACE_SETREGSET request with
NT_X86_XSTATE, while rt_sigreturn() can set them for the current task.
In either case, registers can be set to any value, but the kernel
assumes that the XSAVE area itself remains valid in the sense that the
CPU can restore it.

However, in the case where the kernel is using the uncompacted xstate
format (which it does whenever the XSAVES instruction is unavailable),
it was possible for userspace to set the xcomp_bv field in the
xstate_header to an arbitrary value.  However, all bits in that field
are reserved in the uncompacted 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.  In
addition, since the error is otherwise ignored, the FPU registers from
the task previously executing on the CPU were leaked.

Fix the bug by checking that the user-supplied value of xcomp_bv is 0 in
the uncompacted case, and returning an error otherwise.

The reason for validating xcomp_bv rather than simply overwriting it
with 0 is that we want userspace to see an error if it (incorrectly)
provides an XSAVE area in compacted format rather than in uncompacted
format.

Note that as before, in case of error we clear the task's FPU state.
This is perhaps non-ideal, especially for PTRACE_SETREGSET; it might be
better to return an error before changing anything.  But it seems the
"clear on error" behavior is fine for now, and it's a little tricky to
do otherwise because it would mean we couldn't simply copy the full
userspace state into kernel memory in one __copy_from_user().

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

Here is a C reproducer.  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 ? 0x00000000 : 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;
    }

Note: the program only tests for the bug using the ptrace() system call.
The bug can also be reproduced using the rt_sigreturn() system call, but
only when called from a 32-bit program, since for 64-bit programs the
kernel restores the FPU state from the signal frame by doing XRSTOR
directly from userspace memory (with proper error checking).

Fixes: 0b29643a5843 ("x86/xsaves: Change compacted format xsave area header")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
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 | 9 +++++++--
 arch/x86/kernel/fpu/signal.c | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index b188b16841e3..8ab1a1f4d1c1 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -131,11 +131,16 @@ 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 0 when using uncompacted format */
+		if (!ret && xsave->header.xcomp_bv)
+			ret = -EINVAL;
+	}
+
 	/*
 	 * In case of failure, mark all states as init:
 	 */
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 83c23c230b4c..169d6e001d32 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -329,6 +329,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		} else {
 			err = __copy_from_user(&fpu->state.xsave,
 					       buf_fx, state_size);
+
+			/* xcomp_bv must be 0 when using uncompacted format */
+			if (!err && fpu->state.xsave.header.xcomp_bv)
+				err = -EINVAL;
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
-- 
2.14.1.821.g8fa685d3b7-goog

[kernel-hardening] [PATCH v3 1/3] x86/fpu: don't let userspace set bogus xcomp_bv

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

On x86, userspace can use the ptrace() or rt_sigreturn() system calls to
set a task's extended state (xstate) or "FPU" registers.  ptrace() can
set them for another task using the PTRACE_SETREGSET request with
NT_X86_XSTATE, while rt_sigreturn() can set them for the current task.
In either case, registers can be set to any value, but the kernel
assumes that the XSAVE area itself remains valid in the sense that the
CPU can restore it.

However, in the case where the kernel is using the uncompacted xstate
format (which it does whenever the XSAVES instruction is unavailable),
it was possible for userspace to set the xcomp_bv field in the
xstate_header to an arbitrary value.  However, all bits in that field
are reserved in the uncompacted 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.  In
addition, since the error is otherwise ignored, the FPU registers from
the task previously executing on the CPU were leaked.

Fix the bug by checking that the user-supplied value of xcomp_bv is 0 in
the uncompacted case, and returning an error otherwise.

The reason for validating xcomp_bv rather than simply overwriting it
with 0 is that we want userspace to see an error if it (incorrectly)
provides an XSAVE area in compacted format rather than in uncompacted
format.

Note that as before, in case of error we clear the task's FPU state.
This is perhaps non-ideal, especially for PTRACE_SETREGSET; it might be
better to return an error before changing anything.  But it seems the
"clear on error" behavior is fine for now, and it's a little tricky to
do otherwise because it would mean we couldn't simply copy the full
userspace state into kernel memory in one __copy_from_user().

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

Here is a C reproducer.  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 ? 0x00000000 : 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;
    }

Note: the program only tests for the bug using the ptrace() system call.
The bug can also be reproduced using the rt_sigreturn() system call, but
only when called from a 32-bit program, since for 64-bit programs the
kernel restores the FPU state from the signal frame by doing XRSTOR
directly from userspace memory (with proper error checking).

Fixes: 0b29643a5843 ("x86/xsaves: Change compacted format xsave area header")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
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 | 9 +++++++--
 arch/x86/kernel/fpu/signal.c | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index b188b16841e3..8ab1a1f4d1c1 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -131,11 +131,16 @@ 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 0 when using uncompacted format */
+		if (!ret && xsave->header.xcomp_bv)
+			ret = -EINVAL;
+	}
+
 	/*
 	 * In case of failure, mark all states as init:
 	 */
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 83c23c230b4c..169d6e001d32 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -329,6 +329,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		} else {
 			err = __copy_from_user(&fpu->state.xsave,
 					       buf_fx, state_size);
+
+			/* xcomp_bv must be 0 when using uncompacted format */
+			if (!err && fpu->state.xsave.header.xcomp_bv)
+				err = -EINVAL;
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [kernel-hardening] [PATCH v3 1/3] x86/fpu: don't let userspace set bogus xcomp_bv

[Rik van Riel]

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

On Thu, 2017-09-21 at 11:52 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Fix the bug by checking that the user-supplied value of xcomp_bv is 0
> in
> the uncompacted case, and returning an error otherwise.
> 
Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v3 2/3] x86/fpu: tighten validation of user-supplied xstate_header

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Move validation of user-supplied xstate_headers into a helper function
and call it from both the ptrace and sigreturn syscall paths.  The new
function also considers it to be an error if *any* reserved bits are
set, whereas before we were just clearing most of them.

This should reduce the chance of bugs that fail to correctly validate
user-supplied XSAVE areas.  It also will expose any broken userspace
programs that set the other reserved bits; this is desirable because
such programs will lose compatibility with future CPUs and kernels if
those bits are ever used for anything.  (There shouldn't be any such
programs, and in fact in the case where the compacted format is in use
we were already validating xfeatures.  But you never know...)

Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
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>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/x86/include/asm/fpu/xstate.h | 25 +++++++++++++++++++++++++
 arch/x86/kernel/fpu/regset.c      | 21 +++++++--------------
 arch/x86/kernel/fpu/signal.c      | 19 +++++++++++--------
 arch/x86/kernel/fpu/xstate.c      | 27 ++++++++++-----------------
 4 files changed, 53 insertions(+), 39 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 1b2799e0699a..cf6542b76996 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -52,4 +52,29 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 			void __user *ubuf, struct xregs_state *xsave);
 int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
+
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+static inline int validate_xstate_header(const struct xstate_header *hdr)
+{
+	/* No unknown or supervisor features may be set */
+	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+		return -EINVAL;
+
+	/* Userspace must use the uncompacted format */
+	if (hdr->xcomp_bv)
+		return -EINVAL;
+
+	/*
+	 * If 'reserved' is shrunken to add a new field, make sure to validate
+	 * that new field here!
+	 */
+	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
+
+	/* No reserved bits may be set */
+	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
+		return -EINVAL;
+
+	return 0;
+}
+
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 8ab1a1f4d1c1..3fd3a2f4f591 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -131,31 +131,24 @@ 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 (using_compacted_format()) {
 		ret = copyin_to_xsaves(kbuf, ubuf, xsave);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
-
-		/* xcomp_bv must be 0 when using uncompacted format */
-		if (!ret && xsave->header.xcomp_bv)
-			ret = -EINVAL;
+		if (!ret)
+			ret = validate_xstate_header(&xsave->header);
 	}
 
-	/*
-	 * In case of failure, mark all states as init:
-	 */
-	if (ret)
-		fpstate_init(&fpu->state);
-
 	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->header.xfeatures &= xfeatures_mask;
+
 	/*
-	 * These bits must be zero.
+	 * In case of failure, mark all states as init:
 	 */
-	memset(&xsave->header.reserved, 0, 48);
+	if (ret)
+		fpstate_init(&fpu->state);
 
 	return ret;
 }
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 169d6e001d32..a325f0b25456 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -213,8 +213,11 @@ sanitize_restored_xstate(struct task_struct *tsk,
 	struct xstate_header *header = &xsave->header;
 
 	if (use_xsave()) {
-		/* These bits must be zero. */
-		memset(header->reserved, 0, 48);
+		/*
+		 * Note: we don't need to zero the reserved bits in the
+		 * xstate_header here because we either didn't copy them at all,
+		 * or we checked earlier that they aren't set.
+		 */
 
 		/*
 		 * Init the state that is not present in the memory
@@ -223,7 +226,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
 		if (fx_only)
 			header->xfeatures = XFEATURE_MASK_FPSSE;
 		else
-			header->xfeatures &= (xfeatures_mask & xfeatures);
+			header->xfeatures &= xfeatures;
 	}
 
 	if (use_fxsr()) {
@@ -307,7 +310,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		/*
 		 * For 32-bit frames with fxstate, copy the user state to the
 		 * thread's fpu state, reconstruct fxstate from the fsave
-		 * header. Sanitize the copied state etc.
+		 * header. Validate and sanitize the copied state.
 		 */
 		struct fpu *fpu = &tsk->thread.fpu;
 		struct user_i387_ia32_struct env;
@@ -329,10 +332,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		} else {
 			err = __copy_from_user(&fpu->state.xsave,
 					       buf_fx, state_size);
-
-			/* xcomp_bv must be 0 when using uncompacted format */
-			if (!err && fpu->state.xsave.header.xcomp_bv)
-				err = -EINVAL;
+			if (!err) {
+				err = validate_xstate_header(
+						&fpu->state.xsave.header);
+			}
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c24ac1efb12d..52018be79e27 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1018,41 +1018,34 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 }
 
 /*
- * Convert from a ptrace standard-format buffer to kernel XSAVES format
- * and copy to the target thread. This is called from xstateregs_set() and
- * there we check the CPU has XSAVES and a whole standard-sized buffer
- * exists.
+ * Convert from a ptrace or sigreturn standard-format buffer to kernel XSAVES
+ * format and copy to the target thread.  This is called from xstateregs_set(),
+ * as well as potentially from the sigreturn() and rt_sigreturn() system calls.
  */
 int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave)
 {
 	unsigned int offset, size;
 	int i;
-	u64 xfeatures;
-	u64 allowed_features;
+	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
-	size = sizeof(xfeatures);
+	size = sizeof(hdr);
 
 	if (kbuf) {
-		memcpy(&xfeatures, kbuf + offset, size);
+		memcpy(&hdr, kbuf + offset, size);
 	} else {
-		if (__copy_from_user(&xfeatures, ubuf + offset, size))
+		if (__copy_from_user(&hdr, ubuf + offset, size))
 			return -EFAULT;
 	}
 
-	/*
-	 * Reject if the user sets any disabled or supervisor features:
-	 */
-	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
-
-	if (xfeatures & ~allowed_features)
+	if (validate_xstate_header(&hdr) != 0)
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
 		u64 mask = ((u64)1 << i);
 
-		if (xfeatures & mask) {
+		if (hdr.xfeatures & mask) {
 			void *dst = __raw_xsave_addr(xsave, 1 << i);
 
 			offset = xstate_offsets[i];
@@ -1076,7 +1069,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	/*
 	 * Add back in the features that came in from userspace:
 	 */
-	xsave->header.xfeatures |= xfeatures;
+	xsave->header.xfeatures |= hdr.xfeatures;
 
 	return 0;
 }
-- 
2.14.1.821.g8fa685d3b7-goog

[kernel-hardening] [PATCH v3 2/3] x86/fpu: tighten validation of user-supplied xstate_header

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Move validation of user-supplied xstate_headers into a helper function
and call it from both the ptrace and sigreturn syscall paths.  The new
function also considers it to be an error if *any* reserved bits are
set, whereas before we were just clearing most of them.

This should reduce the chance of bugs that fail to correctly validate
user-supplied XSAVE areas.  It also will expose any broken userspace
programs that set the other reserved bits; this is desirable because
such programs will lose compatibility with future CPUs and kernels if
those bits are ever used for anything.  (There shouldn't be any such
programs, and in fact in the case where the compacted format is in use
we were already validating xfeatures.  But you never know...)

Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
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>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/x86/include/asm/fpu/xstate.h | 25 +++++++++++++++++++++++++
 arch/x86/kernel/fpu/regset.c      | 21 +++++++--------------
 arch/x86/kernel/fpu/signal.c      | 19 +++++++++++--------
 arch/x86/kernel/fpu/xstate.c      | 27 ++++++++++-----------------
 4 files changed, 53 insertions(+), 39 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 1b2799e0699a..cf6542b76996 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -52,4 +52,29 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 			void __user *ubuf, struct xregs_state *xsave);
 int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave);
+
+/* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
+static inline int validate_xstate_header(const struct xstate_header *hdr)
+{
+	/* No unknown or supervisor features may be set */
+	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+		return -EINVAL;
+
+	/* Userspace must use the uncompacted format */
+	if (hdr->xcomp_bv)
+		return -EINVAL;
+
+	/*
+	 * If 'reserved' is shrunken to add a new field, make sure to validate
+	 * that new field here!
+	 */
+	BUILD_BUG_ON(sizeof(hdr->reserved) != 48);
+
+	/* No reserved bits may be set */
+	if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
+		return -EINVAL;
+
+	return 0;
+}
+
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 8ab1a1f4d1c1..3fd3a2f4f591 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -131,31 +131,24 @@ 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 (using_compacted_format()) {
 		ret = copyin_to_xsaves(kbuf, ubuf, xsave);
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
-
-		/* xcomp_bv must be 0 when using uncompacted format */
-		if (!ret && xsave->header.xcomp_bv)
-			ret = -EINVAL;
+		if (!ret)
+			ret = validate_xstate_header(&xsave->header);
 	}
 
-	/*
-	 * In case of failure, mark all states as init:
-	 */
-	if (ret)
-		fpstate_init(&fpu->state);
-
 	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
 	xsave->i387.mxcsr &= mxcsr_feature_mask;
-	xsave->header.xfeatures &= xfeatures_mask;
+
 	/*
-	 * These bits must be zero.
+	 * In case of failure, mark all states as init:
 	 */
-	memset(&xsave->header.reserved, 0, 48);
+	if (ret)
+		fpstate_init(&fpu->state);
 
 	return ret;
 }
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 169d6e001d32..a325f0b25456 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -213,8 +213,11 @@ sanitize_restored_xstate(struct task_struct *tsk,
 	struct xstate_header *header = &xsave->header;
 
 	if (use_xsave()) {
-		/* These bits must be zero. */
-		memset(header->reserved, 0, 48);
+		/*
+		 * Note: we don't need to zero the reserved bits in the
+		 * xstate_header here because we either didn't copy them at all,
+		 * or we checked earlier that they aren't set.
+		 */
 
 		/*
 		 * Init the state that is not present in the memory
@@ -223,7 +226,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
 		if (fx_only)
 			header->xfeatures = XFEATURE_MASK_FPSSE;
 		else
-			header->xfeatures &= (xfeatures_mask & xfeatures);
+			header->xfeatures &= xfeatures;
 	}
 
 	if (use_fxsr()) {
@@ -307,7 +310,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		/*
 		 * For 32-bit frames with fxstate, copy the user state to the
 		 * thread's fpu state, reconstruct fxstate from the fsave
-		 * header. Sanitize the copied state etc.
+		 * header. Validate and sanitize the copied state.
 		 */
 		struct fpu *fpu = &tsk->thread.fpu;
 		struct user_i387_ia32_struct env;
@@ -329,10 +332,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		} else {
 			err = __copy_from_user(&fpu->state.xsave,
 					       buf_fx, state_size);
-
-			/* xcomp_bv must be 0 when using uncompacted format */
-			if (!err && fpu->state.xsave.header.xcomp_bv)
-				err = -EINVAL;
+			if (!err) {
+				err = validate_xstate_header(
+						&fpu->state.xsave.header);
+			}
 		}
 
 		if (err || __copy_from_user(&env, buf, sizeof(env))) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c24ac1efb12d..52018be79e27 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1018,41 +1018,34 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
 }
 
 /*
- * Convert from a ptrace standard-format buffer to kernel XSAVES format
- * and copy to the target thread. This is called from xstateregs_set() and
- * there we check the CPU has XSAVES and a whole standard-sized buffer
- * exists.
+ * Convert from a ptrace or sigreturn standard-format buffer to kernel XSAVES
+ * format and copy to the target thread.  This is called from xstateregs_set(),
+ * as well as potentially from the sigreturn() and rt_sigreturn() system calls.
  */
 int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 		     struct xregs_state *xsave)
 {
 	unsigned int offset, size;
 	int i;
-	u64 xfeatures;
-	u64 allowed_features;
+	struct xstate_header hdr;
 
 	offset = offsetof(struct xregs_state, header);
-	size = sizeof(xfeatures);
+	size = sizeof(hdr);
 
 	if (kbuf) {
-		memcpy(&xfeatures, kbuf + offset, size);
+		memcpy(&hdr, kbuf + offset, size);
 	} else {
-		if (__copy_from_user(&xfeatures, ubuf + offset, size))
+		if (__copy_from_user(&hdr, ubuf + offset, size))
 			return -EFAULT;
 	}
 
-	/*
-	 * Reject if the user sets any disabled or supervisor features:
-	 */
-	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
-
-	if (xfeatures & ~allowed_features)
+	if (validate_xstate_header(&hdr) != 0)
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
 		u64 mask = ((u64)1 << i);
 
-		if (xfeatures & mask) {
+		if (hdr.xfeatures & mask) {
 			void *dst = __raw_xsave_addr(xsave, 1 << i);
 
 			offset = xstate_offsets[i];
@@ -1076,7 +1069,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
 	/*
 	 * Add back in the features that came in from userspace:
 	 */
-	xsave->header.xfeatures |= xfeatures;
+	xsave->header.xfeatures |= hdr.xfeatures;
 
 	return 0;
 }
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [kernel-hardening] [PATCH v3 2/3] x86/fpu: tighten validation of user-supplied xstate_header

[Rik van Riel]

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

On Thu, 2017-09-21 at 11:52 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Move validation of user-supplied xstate_headers into a helper
> function
> and call it from both the ptrace and sigreturn syscall paths.  The
> new
> function also considers it to be an error if *any* reserved bits are
> set, whereas before we were just clearing most of them.
> 
> This should reduce the chance of bugs that fail to correctly validate
> user-supplied XSAVE areas.  It also will expose any broken userspace
> programs that set the other reserved bits; this is desirable because
> such programs will lose compatibility with future CPUs and kernels if
> those bits are ever used for anything.  (There shouldn't be any such
> programs, and in fact in the case where the compacted format is in
> use
> we were already validating xfeatures.  But you never know...)
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> 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>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v3 3/3] x86/fpu: reinitialize FPU registers if restoring FPU state fails

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Userspace can change the FPU state of a task using the ptrace() or
rt_sigreturn() system calls.  Because reserved bits in the FPU state can
cause the XRSTOR instruction to fail, the kernel has to carefully
validate that no reserved bits or other invalid values are being set.

Unfortunately, there have been bugs in this validation code.  For
example, we were not checking that the 'xcomp_bv' field in the
xstate_header was 0.  As-is, such bugs are exploitable to read the FPU
registers of other processes on the system.  To do so, an attacker can
create a task, assign to it an invalid FPU state, then spin in a loop
and monitor the values of the FPU registers.  Because the task's FPU
registers are not being restored, sometimes the FPU registers will have
the values from another process.

This is likely to continue to be a problem in the future because the
validation done by the CPU instructions like XRSTOR is not immediately
visible to kernel developers.  Nor will invalid FPU states ever be
encountered during ordinary use --- they will only be seen during
fuzzing or exploits.  There can even be reserved bits outside the
xstate_header which are easy to forget about.  For example, the MXCSR
register contains reserved bits, which were not validated by the
KVM_SET_XSAVE ioctl until commit a575813bfe4b ("KVM: x86: Fix load
damaged SSEx MXCSR register").

Therefore, mitigate this class of vulnerability by restoring the FPU
registers from init_fpstate if restoring from the task's state fails.

We actually used to do this, but it was (perhaps unwisely) removed by
commit 9ccc27a5d297 ("x86/fpu: Remove error return values from
copy_kernel_to_*regs() functions").  This new patch is also a bit
different.  First, it only clears the registers, not also the bad
in-memory state; this is simpler and makes it easier to make the
mitigation cover all callers of __copy_kernel_to_fpregs().  Second, it
does the register clearing in an exception handler so that no extra
instructions are added to context switches.  In fact, we *remove*
instructions, since previously we were always zeroing the register
containing 'err' even if CONFIG_X86_DEBUG_FPU was disabled.

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>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
 arch/x86/mm/extable.c               | 24 +++++++++++++++++
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 554cdb205d17..719b8cc5fd01 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -120,20 +120,11 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
 	err;								\
 })
 
-#define check_insn(insn, output, input...)				\
-({									\
-	int err;							\
+#define kernel_insn(insn, output, input...)				\
 	asm volatile("1:" #insn "\n\t"					\
 		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:  movl $-1,%[err]\n"				\
-		     "    jmp  2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
-		     : [err] "=r" (err), output				\
-		     : "0"(0), input);					\
-	err;								\
-})
+		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
+		     : output : input)
 
 static inline int copy_fregs_to_user(struct fregs_state __user *fx)
 {
@@ -153,20 +144,16 @@ static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
-	int err;
-
 	if (IS_ENABLED(CONFIG_X86_32)) {
-		err = check_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
+		kernel_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	} else {
 		if (IS_ENABLED(CONFIG_AS_FXSAVEQ)) {
-			err = check_insn(fxrstorq %[fx], "=m" (*fx), [fx] "m" (*fx));
+			kernel_insn(fxrstorq %[fx], "=m" (*fx), [fx] "m" (*fx));
 		} else {
 			/* See comment in copy_fxregs_to_kernel() below. */
-			err = check_insn(rex64/fxrstor (%[fx]), "=m" (*fx), [fx] "R" (fx), "m" (*fx));
+			kernel_insn(rex64/fxrstor (%[fx]), "=m" (*fx), [fx] "R" (fx), "m" (*fx));
 		}
 	}
-	/* Copying from a kernel buffer to FPU registers should never fail: */
-	WARN_ON_FPU(err);
 }
 
 static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
@@ -183,9 +170,7 @@ static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fregs(struct fregs_state *fx)
 {
-	int err = check_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
-
-	WARN_ON_FPU(err);
+	kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_user_to_fregs(struct fregs_state __user *fx)
@@ -281,18 +266,13 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
  * Use XRSTORS to restore context if it is enabled. XRSTORS supports compact
  * XSAVE area format.
  */
-#define XSTATE_XRESTORE(st, lmask, hmask, err)				\
+#define XSTATE_XRESTORE(st, lmask, hmask)				\
 	asm volatile(ALTERNATIVE(XRSTOR,				\
 				 XRSTORS, X86_FEATURE_XSAVES)		\
 		     "\n"						\
-		     "xor %[err], %[err]\n"				\
 		     "3:\n"						\
-		     ".pushsection .fixup,\"ax\"\n"			\
-		     "4: movl $-2, %[err]\n"				\
-		     "jmp 3b\n"						\
-		     ".popsection\n"					\
-		     _ASM_EXTABLE(661b, 4b)				\
-		     : [err] "=r" (err)					\
+		     _ASM_EXTABLE_HANDLE(661b, 3b, ex_handler_fprestore)\
+		     :							\
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")
 
@@ -336,7 +316,10 @@ static inline void copy_kernel_to_xregs_booting(struct xregs_state *xstate)
 	else
 		XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
-	/* We should never fault when copying from a kernel buffer: */
+	/*
+	 * We should never fault when copying from a kernel buffer, and the FPU
+	 * state we set at boot time should be valid.
+	 */
 	WARN_ON_FPU(err);
 }
 
@@ -365,12 +348,8 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 {
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
-	int err;
-
-	XSTATE_XRESTORE(xstate, lmask, hmask, err);
 
-	/* We should never fault when copying from a kernel buffer: */
-	WARN_ON_FPU(err);
+	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
 /*
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c076f710de4c..c3521e2be396 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -2,6 +2,7 @@
 #include <linux/uaccess.h>
 #include <linux/sched/debug.h>
 
+#include <asm/fpu/internal.h>
 #include <asm/traps.h>
 #include <asm/kdebug.h>
 
@@ -78,6 +79,29 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup,
 }
 EXPORT_SYMBOL_GPL(ex_handler_refcount);
 
+/*
+ * Handler for when we fail to restore a task's FPU state.  We should never get
+ * here because the FPU state of a task using the FPU (task->thread.fpu.state)
+ * should always be valid.  However, past bugs have allowed userspace to set
+ * reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn().
+ * These caused XRSTOR to fail when switching to the task, leaking the FPU
+ * registers of the task previously executing on the CPU.  Mitigate this class
+ * of vulnerability by restoring from the initial state (essentially, zeroing
+ * out all the FPU registers) if we can't restore from the task's FPU state.
+ */
+bool ex_handler_fprestore(const struct exception_table_entry *fixup,
+			  struct pt_regs *regs, int trapnr)
+{
+	regs->ip = ex_fixup_addr(fixup);
+
+	WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
+		  (void *)instruction_pointer(regs));
+
+	__copy_kernel_to_fpregs(&init_fpstate, -1);
+	return true;
+}
+EXPORT_SYMBOL_GPL(ex_handler_fprestore);
+
 bool ex_handler_ext(const struct exception_table_entry *fixup,
 		   struct pt_regs *regs, int trapnr)
 {
-- 
2.14.1.821.g8fa685d3b7-goog

[kernel-hardening] [PATCH v3 3/3] x86/fpu: reinitialize FPU registers if restoring FPU state fails

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Userspace can change the FPU state of a task using the ptrace() or
rt_sigreturn() system calls.  Because reserved bits in the FPU state can
cause the XRSTOR instruction to fail, the kernel has to carefully
validate that no reserved bits or other invalid values are being set.

Unfortunately, there have been bugs in this validation code.  For
example, we were not checking that the 'xcomp_bv' field in the
xstate_header was 0.  As-is, such bugs are exploitable to read the FPU
registers of other processes on the system.  To do so, an attacker can
create a task, assign to it an invalid FPU state, then spin in a loop
and monitor the values of the FPU registers.  Because the task's FPU
registers are not being restored, sometimes the FPU registers will have
the values from another process.

This is likely to continue to be a problem in the future because the
validation done by the CPU instructions like XRSTOR is not immediately
visible to kernel developers.  Nor will invalid FPU states ever be
encountered during ordinary use --- they will only be seen during
fuzzing or exploits.  There can even be reserved bits outside the
xstate_header which are easy to forget about.  For example, the MXCSR
register contains reserved bits, which were not validated by the
KVM_SET_XSAVE ioctl until commit a575813bfe4b ("KVM: x86: Fix load
damaged SSEx MXCSR register").

Therefore, mitigate this class of vulnerability by restoring the FPU
registers from init_fpstate if restoring from the task's state fails.

We actually used to do this, but it was (perhaps unwisely) removed by
commit 9ccc27a5d297 ("x86/fpu: Remove error return values from
copy_kernel_to_*regs() functions").  This new patch is also a bit
different.  First, it only clears the registers, not also the bad
in-memory state; this is simpler and makes it easier to make the
mitigation cover all callers of __copy_kernel_to_fpregs().  Second, it
does the register clearing in an exception handler so that no extra
instructions are added to context switches.  In fact, we *remove*
instructions, since previously we were always zeroing the register
containing 'err' even if CONFIG_X86_DEBUG_FPU was disabled.

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>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
 arch/x86/mm/extable.c               | 24 +++++++++++++++++
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 554cdb205d17..719b8cc5fd01 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -120,20 +120,11 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
 	err;								\
 })
 
-#define check_insn(insn, output, input...)				\
-({									\
-	int err;							\
+#define kernel_insn(insn, output, input...)				\
 	asm volatile("1:" #insn "\n\t"					\
 		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:  movl $-1,%[err]\n"				\
-		     "    jmp  2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
-		     : [err] "=r" (err), output				\
-		     : "0"(0), input);					\
-	err;								\
-})
+		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
+		     : output : input)
 
 static inline int copy_fregs_to_user(struct fregs_state __user *fx)
 {
@@ -153,20 +144,16 @@ static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
-	int err;
-
 	if (IS_ENABLED(CONFIG_X86_32)) {
-		err = check_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
+		kernel_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	} else {
 		if (IS_ENABLED(CONFIG_AS_FXSAVEQ)) {
-			err = check_insn(fxrstorq %[fx], "=m" (*fx), [fx] "m" (*fx));
+			kernel_insn(fxrstorq %[fx], "=m" (*fx), [fx] "m" (*fx));
 		} else {
 			/* See comment in copy_fxregs_to_kernel() below. */
-			err = check_insn(rex64/fxrstor (%[fx]), "=m" (*fx), [fx] "R" (fx), "m" (*fx));
+			kernel_insn(rex64/fxrstor (%[fx]), "=m" (*fx), [fx] "R" (fx), "m" (*fx));
 		}
 	}
-	/* Copying from a kernel buffer to FPU registers should never fail: */
-	WARN_ON_FPU(err);
 }
 
 static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
@@ -183,9 +170,7 @@ static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fregs(struct fregs_state *fx)
 {
-	int err = check_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
-
-	WARN_ON_FPU(err);
+	kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_user_to_fregs(struct fregs_state __user *fx)
@@ -281,18 +266,13 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
  * Use XRSTORS to restore context if it is enabled. XRSTORS supports compact
  * XSAVE area format.
  */
-#define XSTATE_XRESTORE(st, lmask, hmask, err)				\
+#define XSTATE_XRESTORE(st, lmask, hmask)				\
 	asm volatile(ALTERNATIVE(XRSTOR,				\
 				 XRSTORS, X86_FEATURE_XSAVES)		\
 		     "\n"						\
-		     "xor %[err], %[err]\n"				\
 		     "3:\n"						\
-		     ".pushsection .fixup,\"ax\"\n"			\
-		     "4: movl $-2, %[err]\n"				\
-		     "jmp 3b\n"						\
-		     ".popsection\n"					\
-		     _ASM_EXTABLE(661b, 4b)				\
-		     : [err] "=r" (err)					\
+		     _ASM_EXTABLE_HANDLE(661b, 3b, ex_handler_fprestore)\
+		     :							\
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")
 
@@ -336,7 +316,10 @@ static inline void copy_kernel_to_xregs_booting(struct xregs_state *xstate)
 	else
 		XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
-	/* We should never fault when copying from a kernel buffer: */
+	/*
+	 * We should never fault when copying from a kernel buffer, and the FPU
+	 * state we set at boot time should be valid.
+	 */
 	WARN_ON_FPU(err);
 }
 
@@ -365,12 +348,8 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 {
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
-	int err;
-
-	XSTATE_XRESTORE(xstate, lmask, hmask, err);
 
-	/* We should never fault when copying from a kernel buffer: */
-	WARN_ON_FPU(err);
+	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
 /*
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c076f710de4c..c3521e2be396 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -2,6 +2,7 @@
 #include <linux/uaccess.h>
 #include <linux/sched/debug.h>
 
+#include <asm/fpu/internal.h>
 #include <asm/traps.h>
 #include <asm/kdebug.h>
 
@@ -78,6 +79,29 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup,
 }
 EXPORT_SYMBOL_GPL(ex_handler_refcount);
 
+/*
+ * Handler for when we fail to restore a task's FPU state.  We should never get
+ * here because the FPU state of a task using the FPU (task->thread.fpu.state)
+ * should always be valid.  However, past bugs have allowed userspace to set
+ * reserved bits in the XSAVE area using PTRACE_SETREGSET or sys_rt_sigreturn().
+ * These caused XRSTOR to fail when switching to the task, leaking the FPU
+ * registers of the task previously executing on the CPU.  Mitigate this class
+ * of vulnerability by restoring from the initial state (essentially, zeroing
+ * out all the FPU registers) if we can't restore from the task's FPU state.
+ */
+bool ex_handler_fprestore(const struct exception_table_entry *fixup,
+			  struct pt_regs *regs, int trapnr)
+{
+	regs->ip = ex_fixup_addr(fixup);
+
+	WARN_ONCE(1, "Bad FPU state detected at %pB, reinitializing FPU registers.",
+		  (void *)instruction_pointer(regs));
+
+	__copy_kernel_to_fpregs(&init_fpstate, -1);
+	return true;
+}
+EXPORT_SYMBOL_GPL(ex_handler_fprestore);
+
 bool ex_handler_ext(const struct exception_table_entry *fixup,
 		   struct pt_regs *regs, int trapnr)
 {
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [kernel-hardening] [PATCH v3 3/3] x86/fpu: reinitialize FPU registers if restoring FPU state fails

[Rik van Riel]

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

On Thu, 2017-09-21 at 11:52 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Userspace can change the FPU state of a task using the ptrace() or
> rt_sigreturn() system calls.  Because reserved bits in the FPU state
> can
> cause the XRSTOR instruction to fail, the kernel has to carefully
> validate that no reserved bits or other invalid values are being set.
> 
> Unfortunately, there have been bugs in this validation code.  For
> example, we were not checking that the 'xcomp_bv' field in the
> xstate_header was 0.  As-is, such bugs are exploitable to read the
> FPU
> registers of other processes on the system.  To do so, an attacker
> can
> create a task, assign to it an invalid FPU state, then spin in a loop
> and monitor the values of the FPU registers.  Because the task's FPU
> registers are not being restored, sometimes the FPU registers will
> have
> the values from another process.
> 

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Ingo Molnar]

* Eric Biggers <ebiggers3@gmail.com> wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> This series fixes the bug found by syzkaller where the ptrace syscall
> can be used to set invalid bits in a task's FPU state.  I also found
> that an equivalent bug was reachable using the sigreturn syscall, so the
> first patch fixes the bug in both cases.
> 
> The other two patches start validating the other parts of the
> xstate_header and make it so that invalid FPU states can no longer be
> abused to leak the FPU registers of other processes.
> 
> Changes since v2:
>     - Use an exception handler to handle invalid FPU states
>       (suggested by Andy Lutomirski)
>     - Check the size of xstate_header.reserved at build time
>       (suggested by Dave Hansen)
> 
> Eric Biggers (3):
>   x86/fpu: don't let userspace set bogus xcomp_bv
>   x86/fpu: tighten validation of user-supplied xstate_header
>   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> 
>  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
>  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
>  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
>  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
>  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
>  arch/x86/mm/extable.c               | 24 +++++++++++++++++
>  6 files changed, 94 insertions(+), 68 deletions(-)

Ok - could you please rebase these to to tip:master that is at:

	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master

In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
not merged upstream (yet), which conflict with these changes. I'd like to merge 
them all together.

Thanks,

	Ingo

[kernel-hardening] Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Ingo Molnar]

* Eric Biggers <ebiggers3@gmail.com> wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> This series fixes the bug found by syzkaller where the ptrace syscall
> can be used to set invalid bits in a task's FPU state.  I also found
> that an equivalent bug was reachable using the sigreturn syscall, so the
> first patch fixes the bug in both cases.
> 
> The other two patches start validating the other parts of the
> xstate_header and make it so that invalid FPU states can no longer be
> abused to leak the FPU registers of other processes.
> 
> Changes since v2:
>     - Use an exception handler to handle invalid FPU states
>       (suggested by Andy Lutomirski)
>     - Check the size of xstate_header.reserved at build time
>       (suggested by Dave Hansen)
> 
> Eric Biggers (3):
>   x86/fpu: don't let userspace set bogus xcomp_bv
>   x86/fpu: tighten validation of user-supplied xstate_header
>   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> 
>  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
>  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
>  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
>  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
>  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
>  arch/x86/mm/extable.c               | 24 +++++++++++++++++
>  6 files changed, 94 insertions(+), 68 deletions(-)

Ok - could you please rebase these to to tip:master that is at:

	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master

In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
not merged upstream (yet), which conflict with these changes. I'd like to merge 
them all together.

Thanks,

	Ingo

Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Eric Biggers]

On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> 
> * Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > This series fixes the bug found by syzkaller where the ptrace syscall
> > can be used to set invalid bits in a task's FPU state.  I also found
> > that an equivalent bug was reachable using the sigreturn syscall, so the
> > first patch fixes the bug in both cases.
> > 
> > The other two patches start validating the other parts of the
> > xstate_header and make it so that invalid FPU states can no longer be
> > abused to leak the FPU registers of other processes.
> > 
> > Changes since v2:
> >     - Use an exception handler to handle invalid FPU states
> >       (suggested by Andy Lutomirski)
> >     - Check the size of xstate_header.reserved at build time
> >       (suggested by Dave Hansen)
> > 
> > Eric Biggers (3):
> >   x86/fpu: don't let userspace set bogus xcomp_bv
> >   x86/fpu: tighten validation of user-supplied xstate_header
> >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > 
> >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> >  6 files changed, 94 insertions(+), 68 deletions(-)
> 
> Ok - could you please rebase these to to tip:master that is at:
> 
> 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> 
> In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> not merged upstream (yet), which conflict with these changes. I'd like to merge 
> them all together.
> 

Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
causing the following warning:

[    5.024462] WARNING: CPU: 1 PID: 347 at arch/x86/kernel/fpu/core.c:147 fpu__save+0x2ab/0x2c0
[    5.025030] CPU: 1 PID: 347 Comm: reproduce Not tainted 4.14.0-rc1-00274-ga4cc07ed56ab #615
[    5.025639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
[    5.026183] task: ffff8af778664380 task.stack: ffff98f6008f8000
[    5.026590] RIP: 0010:fpu__save+0x2ab/0x2c0
[    5.027376] RSP: 0018:ffff98f6008fbd90 EFLAGS: 00010202
[    5.027751] RAX: ffff8af778665a40 RBX: ffff8af779ccfb80 RCX: 0000000000000340
[    5.028235] RDX: 0000000000000000 RSI: ffffffffb566d348 RDI: ffff8af779ccfb80
[    5.028718] RBP: ffff98f6008fbda0 R08: 0000000000000000 R09: 00007fffb907e870
[    5.029205] R10: ffffffffb4a25780 R11: ffffffffb5404c60 R12: ffff8af779ccfb80
[    5.029685] R13: 0000000000000340 R14: ffff8af779cce4c0 R15: ffff8af779ccfb80
[    5.030557] FS:  00007f662220d700(0000) GS:ffff8af77f400000(0000) knlGS:0000000000000000
[    5.031114] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    5.031504] CR2: 00007fa712274008 CR3: 0000000078730003 CR4: 00000000001606e0
[    5.031990] Call Trace:
[    5.032166]  fpu__activate_fpstate_read+0x90/0x240
[    5.032493]  xstateregs_get+0x53/0x160
[    5.032767]  ptrace_regset+0x123/0x180
[    5.033034]  ptrace_request+0x460/0x590
[    5.033300]  ? wait_task_inactive+0xdb/0x270
[    5.043352]  arch_ptrace+0x34c/0x3d0
[    5.043631]  ? ptrace_check_attach+0xd1/0x120
[    5.043929]  SyS_ptrace+0xa6/0x100
[    5.044164]  entry_SYSCALL_64_fastpath+0x1f/0xbe
[    5.044478] RIP: 0033:0x7f6621b17c6e
[    5.044724] RSP: 002b:00007fffb907e838 EFLAGS: 00000202 ORIG_RAX: 0000000000000065
[    5.045233] RAX: ffffffffffffffda RBX: 0000000000601050 RCX: 00007f6621b17c6e
[    5.045711] RDX: 0000000000000202 RSI: 000000000000015c RDI: 0000000000004204
[    5.046189] RBP: 00000000004008e0 R08: 0000000000004203 R09: 000000000000015b
[    5.046668] R10: 00007fffb907e850 R11: 0000000000000202 R12: 00000000004007f2
[    5.047777] R13: 00007fffb907f960 R14: 0000000000000000 R15: 0000000000000000
[    5.048262] Code: c0 0f 85 ce fe ff ff 48 c7 c2 88 34 5c b5 be 2c 00 00 00 48 c7 c7 e0 5f 5c b5 c6 05 49 71 f0 00 01 e8 da ae 0a 00 e9 aa fe ff ff <0f> ff e9 75 fd ff ff 0f ff e9 b5 fd ff ff 0f 1f 80 00 00 00 00 

[kernel-hardening] Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state

[Eric Biggers]

On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> 
> * Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > This series fixes the bug found by syzkaller where the ptrace syscall
> > can be used to set invalid bits in a task's FPU state.  I also found
> > that an equivalent bug was reachable using the sigreturn syscall, so the
> > first patch fixes the bug in both cases.
> > 
> > The other two patches start validating the other parts of the
> > xstate_header and make it so that invalid FPU states can no longer be
> > abused to leak the FPU registers of other processes.
> > 
> > Changes since v2:
> >     - Use an exception handler to handle invalid FPU states
> >       (suggested by Andy Lutomirski)
> >     - Check the size of xstate_header.reserved at build time
> >       (suggested by Dave Hansen)
> > 
> > Eric Biggers (3):
> >   x86/fpu: don't let userspace set bogus xcomp_bv
> >   x86/fpu: tighten validation of user-supplied xstate_header
> >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > 
> >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> >  6 files changed, 94 insertions(+), 68 deletions(-)
> 
> Ok - could you please rebase these to to tip:master that is at:
> 
> 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> 
> In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> not merged upstream (yet), which conflict with these changes. I'd like to merge 
> them all together.
> 

Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
causing the following warning:

[    5.024462] WARNING: CPU: 1 PID: 347 at arch/x86/kernel/fpu/core.c:147 fpu__save+0x2ab/0x2c0
[    5.025030] CPU: 1 PID: 347 Comm: reproduce Not tainted 4.14.0-rc1-00274-ga4cc07ed56ab #615
[    5.025639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
[    5.026183] task: ffff8af778664380 task.stack: ffff98f6008f8000
[    5.026590] RIP: 0010:fpu__save+0x2ab/0x2c0
[    5.027376] RSP: 0018:ffff98f6008fbd90 EFLAGS: 00010202
[    5.027751] RAX: ffff8af778665a40 RBX: ffff8af779ccfb80 RCX: 0000000000000340
[    5.028235] RDX: 0000000000000000 RSI: ffffffffb566d348 RDI: ffff8af779ccfb80
[    5.028718] RBP: ffff98f6008fbda0 R08: 0000000000000000 R09: 00007fffb907e870
[    5.029205] R10: ffffffffb4a25780 R11: ffffffffb5404c60 R12: ffff8af779ccfb80
[    5.029685] R13: 0000000000000340 R14: ffff8af779cce4c0 R15: ffff8af779ccfb80
[    5.030557] FS:  00007f662220d700(0000) GS:ffff8af77f400000(0000) knlGS:0000000000000000
[    5.031114] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    5.031504] CR2: 00007fa712274008 CR3: 0000000078730003 CR4: 00000000001606e0
[    5.031990] Call Trace:
[    5.032166]  fpu__activate_fpstate_read+0x90/0x240
[    5.032493]  xstateregs_get+0x53/0x160
[    5.032767]  ptrace_regset+0x123/0x180
[    5.033034]  ptrace_request+0x460/0x590
[    5.033300]  ? wait_task_inactive+0xdb/0x270
[    5.043352]  arch_ptrace+0x34c/0x3d0
[    5.043631]  ? ptrace_check_attach+0xd1/0x120
[    5.043929]  SyS_ptrace+0xa6/0x100
[    5.044164]  entry_SYSCALL_64_fastpath+0x1f/0xbe
[    5.044478] RIP: 0033:0x7f6621b17c6e
[    5.044724] RSP: 002b:00007fffb907e838 EFLAGS: 00000202 ORIG_RAX: 0000000000000065
[    5.045233] RAX: ffffffffffffffda RBX: 0000000000601050 RCX: 00007f6621b17c6e
[    5.045711] RDX: 0000000000000202 RSI: 000000000000015c RDI: 0000000000004204
[    5.046189] RBP: 00000000004008e0 R08: 0000000000004203 R09: 000000000000015b
[    5.046668] R10: 00007fffb907e850 R11: 0000000000000202 R12: 00000000004007f2
[    5.047777] R13: 00007fffb907f960 R14: 0000000000000000 R15: 0000000000000000
[    5.048262] Code: c0 0f 85 ce fe ff ff 48 c7 c2 88 34 5c b5 be 2c 00 00 00 48 c7 c7 e0 5f 5c b5 c6 05 49 71 f0 00 01 e8 da ae 0a 00 e9 aa fe ff ff <0f> ff e9 75 fd ff ff 0f ff e9 b5 fd ff ff 0f 1f 80 00 00 00 00 

[PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Ingo Molnar]

* Eric Biggers <ebiggers3@gmail.com> wrote:

> On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> > 
> > * Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > This series fixes the bug found by syzkaller where the ptrace syscall
> > > can be used to set invalid bits in a task's FPU state.  I also found
> > > that an equivalent bug was reachable using the sigreturn syscall, so the
> > > first patch fixes the bug in both cases.
> > > 
> > > The other two patches start validating the other parts of the
> > > xstate_header and make it so that invalid FPU states can no longer be
> > > abused to leak the FPU registers of other processes.
> > > 
> > > Changes since v2:
> > >     - Use an exception handler to handle invalid FPU states
> > >       (suggested by Andy Lutomirski)
> > >     - Check the size of xstate_header.reserved at build time
> > >       (suggested by Dave Hansen)
> > > 
> > > Eric Biggers (3):
> > >   x86/fpu: don't let userspace set bogus xcomp_bv
> > >   x86/fpu: tighten validation of user-supplied xstate_header
> > >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > > 
> > >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> > >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> > >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> > >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> > >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> > >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> > >  6 files changed, 94 insertions(+), 68 deletions(-)
> > 
> > Ok - could you please rebase these to to tip:master that is at:
> > 
> > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > 
> > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > them all together.
> > 
> 
> Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> causing the following warning:

Yes, the warning should be harmless, and I fixed it locally earlier today - does 
the patch below solve it for you as well?

Thanks,

	Ingo

============================>
Subject: x86/fpu: Simplify fpu__activate_fpstate_read()
From: Ingo Molnar <mingo@kernel.org>
Date: Sat Sep 23 11:41:23 CEST 2017

fpu__activate_fpstate_read() can only ever be called for a non-current,
non-executing (stopped) task - so make sure this is checked via a warning
and remove the current-task logic.

This also fixes an incorrect (but harmless) warning triggered by of the earlier 
patches.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/core.c |   24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

Index: linux/arch/x86/kernel/fpu/core.c
===================================================================
--- linux.orig/arch/x86/kernel/fpu/core.c
+++ linux/arch/x86/kernel/fpu/core.c
@@ -256,26 +256,18 @@ EXPORT_SYMBOL_GPL(fpu__activate_curr);
  *
  * If the task has not used the FPU before then initialize its
  * fpstate.
- *
- * If the task has used the FPU before then save it.
  */
 void fpu__activate_fpstate_read(struct fpu *fpu)
 {
-	/*
-	 * If fpregs are active (in the current CPU), then
-	 * copy them to the fpstate:
-	 */
-	if (fpu->fpstate_active) {
-		fpu__save(fpu);
-	} else {
-		if (!fpu->fpstate_active) {
-			fpstate_init(&fpu->state);
-			trace_x86_fpu_init_state(fpu);
+	WARN_ON_FPU(fpu == &current->thread.fpu);
+
+	if (!fpu->fpstate_active) {
+		fpstate_init(&fpu->state);
+		trace_x86_fpu_init_state(fpu);
 
-			trace_x86_fpu_activate_state(fpu);
-			/* Safe to do for current and for stopped child tasks: */
-			fpu->fpstate_active = 1;
-		}
+		trace_x86_fpu_activate_state(fpu);
+		/* Safe to do for current and for stopped child tasks: */
+		fpu->fpstate_active = 1;
 	}
 }
 

[kernel-hardening] [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Ingo Molnar]

* Eric Biggers <ebiggers3@gmail.com> wrote:

> On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> > 
> > * Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > This series fixes the bug found by syzkaller where the ptrace syscall
> > > can be used to set invalid bits in a task's FPU state.  I also found
> > > that an equivalent bug was reachable using the sigreturn syscall, so the
> > > first patch fixes the bug in both cases.
> > > 
> > > The other two patches start validating the other parts of the
> > > xstate_header and make it so that invalid FPU states can no longer be
> > > abused to leak the FPU registers of other processes.
> > > 
> > > Changes since v2:
> > >     - Use an exception handler to handle invalid FPU states
> > >       (suggested by Andy Lutomirski)
> > >     - Check the size of xstate_header.reserved at build time
> > >       (suggested by Dave Hansen)
> > > 
> > > Eric Biggers (3):
> > >   x86/fpu: don't let userspace set bogus xcomp_bv
> > >   x86/fpu: tighten validation of user-supplied xstate_header
> > >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > > 
> > >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> > >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> > >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> > >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> > >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> > >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> > >  6 files changed, 94 insertions(+), 68 deletions(-)
> > 
> > Ok - could you please rebase these to to tip:master that is at:
> > 
> > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > 
> > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > them all together.
> > 
> 
> Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> causing the following warning:

Yes, the warning should be harmless, and I fixed it locally earlier today - does 
the patch below solve it for you as well?

Thanks,

	Ingo

============================>
Subject: x86/fpu: Simplify fpu__activate_fpstate_read()
From: Ingo Molnar <mingo@kernel.org>
Date: Sat Sep 23 11:41:23 CEST 2017

fpu__activate_fpstate_read() can only ever be called for a non-current,
non-executing (stopped) task - so make sure this is checked via a warning
and remove the current-task logic.

This also fixes an incorrect (but harmless) warning triggered by of the earlier 
patches.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/fpu/core.c |   24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

Index: linux/arch/x86/kernel/fpu/core.c
===================================================================
--- linux.orig/arch/x86/kernel/fpu/core.c
+++ linux/arch/x86/kernel/fpu/core.c
@@ -256,26 +256,18 @@ EXPORT_SYMBOL_GPL(fpu__activate_curr);
  *
  * If the task has not used the FPU before then initialize its
  * fpstate.
- *
- * If the task has used the FPU before then save it.
  */
 void fpu__activate_fpstate_read(struct fpu *fpu)
 {
-	/*
-	 * If fpregs are active (in the current CPU), then
-	 * copy them to the fpstate:
-	 */
-	if (fpu->fpstate_active) {
-		fpu__save(fpu);
-	} else {
-		if (!fpu->fpstate_active) {
-			fpstate_init(&fpu->state);
-			trace_x86_fpu_init_state(fpu);
+	WARN_ON_FPU(fpu == &current->thread.fpu);
+
+	if (!fpu->fpstate_active) {
+		fpstate_init(&fpu->state);
+		trace_x86_fpu_init_state(fpu);
 
-			trace_x86_fpu_activate_state(fpu);
-			/* Safe to do for current and for stopped child tasks: */
-			fpu->fpstate_active = 1;
-		}
+		trace_x86_fpu_activate_state(fpu);
+		/* Safe to do for current and for stopped child tasks: */
+		fpu->fpstate_active = 1;
 	}
 }
 

Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> > > 
> > > * Eric Biggers <ebiggers3@gmail.com> wrote:
> > > 
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > This series fixes the bug found by syzkaller where the ptrace syscall
> > > > can be used to set invalid bits in a task's FPU state.  I also found
> > > > that an equivalent bug was reachable using the sigreturn syscall, so the
> > > > first patch fixes the bug in both cases.
> > > > 
> > > > The other two patches start validating the other parts of the
> > > > xstate_header and make it so that invalid FPU states can no longer be
> > > > abused to leak the FPU registers of other processes.
> > > > 
> > > > Changes since v2:
> > > >     - Use an exception handler to handle invalid FPU states
> > > >       (suggested by Andy Lutomirski)
> > > >     - Check the size of xstate_header.reserved at build time
> > > >       (suggested by Dave Hansen)
> > > > 
> > > > Eric Biggers (3):
> > > >   x86/fpu: don't let userspace set bogus xcomp_bv
> > > >   x86/fpu: tighten validation of user-supplied xstate_header
> > > >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > > > 
> > > >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> > > >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> > > >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> > > >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> > > >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> > > >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> > > >  6 files changed, 94 insertions(+), 68 deletions(-)
> > > 
> > > Ok - could you please rebase these to to tip:master that is at:
> > > 
> > > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > > 
> > > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > > them all together.
> > > 
> > 
> > Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> > causing the following warning:
> 
> Yes, the warning should be harmless, and I fixed it locally earlier today - does 
> the patch below solve it for you as well?

Note that this fix is now part of tip:master as well, so if you re-test -tip you 
should get all the latest fixes as well (including yours!).

Thanks,

	Ingo

[kernel-hardening] Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > On Fri, Sep 22, 2017 at 07:33:14AM +0200, Ingo Molnar wrote:
> > > 
> > > * Eric Biggers <ebiggers3@gmail.com> wrote:
> > > 
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > This series fixes the bug found by syzkaller where the ptrace syscall
> > > > can be used to set invalid bits in a task's FPU state.  I also found
> > > > that an equivalent bug was reachable using the sigreturn syscall, so the
> > > > first patch fixes the bug in both cases.
> > > > 
> > > > The other two patches start validating the other parts of the
> > > > xstate_header and make it so that invalid FPU states can no longer be
> > > > abused to leak the FPU registers of other processes.
> > > > 
> > > > Changes since v2:
> > > >     - Use an exception handler to handle invalid FPU states
> > > >       (suggested by Andy Lutomirski)
> > > >     - Check the size of xstate_header.reserved at build time
> > > >       (suggested by Dave Hansen)
> > > > 
> > > > Eric Biggers (3):
> > > >   x86/fpu: don't let userspace set bogus xcomp_bv
> > > >   x86/fpu: tighten validation of user-supplied xstate_header
> > > >   x86/fpu: reinitialize FPU registers if restoring FPU state fails
> > > > 
> > > >  arch/x86/include/asm/fpu/internal.h | 51 +++++++++++--------------------------
> > > >  arch/x86/include/asm/fpu/xstate.h   | 25 ++++++++++++++++++
> > > >  arch/x86/kernel/fpu/regset.c        | 20 +++++++--------
> > > >  arch/x86/kernel/fpu/signal.c        | 15 ++++++++---
> > > >  arch/x86/kernel/fpu/xstate.c        | 27 ++++++++------------
> > > >  arch/x86/mm/extable.c               | 24 +++++++++++++++++
> > > >  6 files changed, 94 insertions(+), 68 deletions(-)
> > > 
> > > Ok - could you please rebase these to to tip:master that is at:
> > > 
> > > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > > 
> > > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > > them all together.
> > > 
> > 
> > Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> > causing the following warning:
> 
> Yes, the warning should be harmless, and I fixed it locally earlier today - does 
> the patch below solve it for you as well?

Note that this fix is now part of tip:master as well, so if you re-test -tip you 
should get all the latest fixes as well (including yours!).

Thanks,

	Ingo

Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Eric Biggers]

On Sat, Sep 23, 2017 at 01:29:32PM +0200, Ingo Molnar wrote:
> > > > 
> > > > Ok - could you please rebase these to to tip:master that is at:
> > > > 
> > > > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > > > 
> > > > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > > > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > > > them all together.
> > > > 
> > > 
> > > Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> > > causing the following warning:
> > 
> > Yes, the warning should be harmless, and I fixed it locally earlier today - does 
> > the patch below solve it for you as well?
> 
> Note that this fix is now part of tip:master as well, so if you re-test -tip you 
> should get all the latest fixes as well (including yours!).
> 

No issues so far with the latest tip/master (commit e7c6e3675331).

Eric

[kernel-hardening] Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

[Eric Biggers]

On Sat, Sep 23, 2017 at 01:29:32PM +0200, Ingo Molnar wrote:
> > > > 
> > > > Ok - could you please rebase these to to tip:master that is at:
> > > > 
> > > > 	git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master
> > > > 
> > > > In particular this has a WIP.x86/fpu branch with FPU fixes+changes queued up but 
> > > > not merged upstream (yet), which conflict with these changes. I'd like to merge 
> > > > them all together.
> > > > 
> > > 
> > > Working on it, but there is a problem with current tip.  PTRACE_GETREGSET is
> > > causing the following warning:
> > 
> > Yes, the warning should be harmless, and I fixed it locally earlier today - does 
> > the patch below solve it for you as well?
> 
> Note that this fix is now part of tip:master as well, so if you re-test -tip you 
> should get all the latest fixes as well (including yours!).
> 

No issues so far with the latest tip/master (commit e7c6e3675331).

Eric