Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1735376 > unrolled thread
| Started by | Eric Biggers <ebiggers3@gmail.com> |
|---|---|
| First post | 2017-09-20 02:50 +0200 |
| Last post | 2017-09-20 05:10 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state Eric Biggers <ebiggers3@gmail.com> - 2017-09-20 02:50 +0200
[PATCH v2 1/3] x86/fpu: don't let userspace set bogus xcomp_bv Eric Biggers <ebiggers3@gmail.com> - 2017-09-20 02:50 +0200
Re: [lkp-robot] [x86/fpu] 14e633085a: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Eric Biggers <ebiggers3@gmail.com> - 2017-09-26 05:10 +0200
Re: [PATCH v2 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state Kees Cook <keescook@chromium.org> - 2017-09-20 05:10 +0200
| From | Eric Biggers <ebiggers3@gmail.com> |
|---|---|
| Date | 2017-09-20 02:50 +0200 |
| Subject | [PATCH v2 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state |
| Message-ID | <urBnb-42V-5@gated-at.bofh.it> |
From: Eric Biggers <ebiggers@google.com> This is a second attempt to fix 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. 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 | 29 ++++++++++++++++++++--------- arch/x86/include/asm/fpu/xstate.h | 19 +++++++++++++++++++ arch/x86/kernel/fpu/core.c | 16 ++++++++++++++++ arch/x86/kernel/fpu/regset.c | 20 +++++++++----------- arch/x86/kernel/fpu/signal.c | 15 +++++++++++---- arch/x86/kernel/fpu/xstate.c | 27 ++++++++++----------------- 6 files changed, 85 insertions(+), 41 deletions(-) -- 2.14.1.690.gbb1197296e-goog
[toc] | [next] | [standalone]
| From | Eric Biggers <ebiggers3@gmail.com> |
|---|---|
| Date | 2017-09-20 02:50 +0200 |
| Subject | [PATCH v2 1/3] x86/fpu: don't let userspace set bogus xcomp_bv |
| Message-ID | <urBnc-42V-9@gated-at.bofh.it> |
| In reply to | #1735376 |
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 any 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>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hao <haokexin@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: <stable@vger.kernel.org> [v3.17+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
arch/x86/kernel/fpu/regset.c | 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.690.gbb1197296e-goog
[toc] | [prev] | [next] | [standalone]
| From | Eric Biggers <ebiggers3@gmail.com> |
|---|---|
| Date | 2017-09-26 05:10 +0200 |
| Subject | Re: [lkp-robot] [x86/fpu] 14e633085a: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= |
| Message-ID | <utOpX-7kB-1@gated-at.bofh.it> |
| In reply to | #1735377 |
On Tue, Sep 26, 2017 at 10:15:36AM +0800, kernel test robot wrote: > [ 7.286014] init[1] bad frame in 32bit sigreturn frame:00000000fff5eb2c ip:f7f369b5 sp:fff5f08c orax:ffffffffffffffff in libuClibc-0.9.33.2.so[f7f2c000+4f000] > [ 7.288482] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b > [ 7.288482] > [ 7.289898] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc1-00021-g14e6330 #38 > [ 7.290988] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014 > [ 7.292562] Call Trace: > [ 7.292960] dump_stack+0x61/0x7e > [ 7.293485] panic+0xd3/0x20f > [ 7.293967] do_exit+0x4f2/0x983 > [ 7.294440] do_group_exit+0x45/0xb0 > [ 7.294966] get_signal+0x4b8/0x4e4 > [ 7.295483] do_signal+0x23/0x5bc > [ 7.295964] ? force_sig_info+0xc6/0xd5 > [ 7.296521] ? force_sig+0x11/0x13 > [ 7.297027] ? signal_fault+0xb8/0xc1 > [ 7.297557] exit_to_usermode_loop+0x3a/0x72 > [ 7.298178] do_int80_syscall_32+0xe9/0xfe > [ 7.298784] entry_INT80_compat+0x2a/0x40 > [ 7.299374] RIP: 0023:0xf7f369b5 > [ 7.299848] RSP: 002b:00000000fff5f08c EFLAGS: 00000246 > [ 7.300587] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 00000000f7fb670c > [ 7.301609] RDX: 000000000000000a RSI: 0000000000000f9f RDI: 0000000000000fa0 > [ 7.302624] RBP: 00000000fff5f0f8 R08: 0000000000000000 R09: 0000000000000000 > [ 7.303631] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > [ 7.304666] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 > [ 7.305677] Kernel Offset: 0x7000000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff) > > Elapsed time: 10 Thanks for testing! This should already be fixed by the version of the patch in tip/master (commit 814fb7bb7db543); there was a missing check for 'state_size'. Eric
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-09-20 05:10 +0200 |
| Subject | Re: [PATCH v2 0/3] x86/fpu: prevent leaking FPU registers via invalid FPU state |
| Message-ID | <urDyG-5Fl-9@gated-at.bofh.it> |
| In reply to | #1735376 |
On Tue, Sep 19, 2017 at 5:44 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > From: Eric Biggers <ebiggers@google.com> > > This is a second attempt to fix 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. > > 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 This series looks sensible to me! Thanks for getting this fixed up. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > arch/x86/include/asm/fpu/internal.h | 29 ++++++++++++++++++++--------- > arch/x86/include/asm/fpu/xstate.h | 19 +++++++++++++++++++ > arch/x86/kernel/fpu/core.c | 16 ++++++++++++++++ > arch/x86/kernel/fpu/regset.c | 20 +++++++++----------- > arch/x86/kernel/fpu/signal.c | 15 +++++++++++---- > arch/x86/kernel/fpu/xstate.c | 27 ++++++++++----------------- > 6 files changed, 85 insertions(+), 41 deletions(-) > > -- > 2.14.1.690.gbb1197296e-goog > -- Kees Cook Pixel Security
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web