Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1737012 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state Eric Biggers <ebiggers3@gmail.com> - 2017-09-21 21:00 +0200
    [PATCH v3 1/3] x86/fpu: don't let userspace set bogus xcomp_bv Eric Biggers <ebiggers3@gmail.com> - 2017-09-21 21:00 +0200
      Re: [kernel-hardening] [PATCH v3 1/3] x86/fpu: don't let userspace  set bogus xcomp_bv Rik van Riel <riel@redhat.com> - 2017-09-21 22:00 +0200
    [PATCH v3 2/3] x86/fpu: tighten validation of user-supplied xstate_header Eric Biggers <ebiggers3@gmail.com> - 2017-09-21 21:00 +0200
      Re: [kernel-hardening] [PATCH v3 2/3] x86/fpu: tighten validation  of user-supplied xstate_header Rik van Riel <riel@redhat.com> - 2017-09-21 22:30 +0200
    Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via  invalid FPU state Ingo Molnar <mingo@kernel.org> - 2017-09-22 07:40 +0200
      Re: [PATCH v3 0/3] x86/fpu: prevent leaking FPU registers via  invalid FPU state Eric Biggers <ebiggers3@gmail.com> - 2017-09-22 19:10 +0200
        [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read() Ingo Molnar <mingo@kernel.org> - 2017-09-23 12:20 +0200
          Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read() Ingo Molnar <mingo@kernel.org> - 2017-09-23 13:30 +0200
            Re: [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read() Eric Biggers <ebiggers3@gmail.com> - 2017-09-23 20:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

* 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

[toc] | [prev] | [next] | [standalone]

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

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 

[toc] | [prev] | [next] | [standalone]

#1737987 — [PATCH] x86/fpu: Simplify fpu__activate_fpstate_read()

* 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;
 	}
 }
 

[toc] | [prev] | [next] | [standalone]

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

* 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web