From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030251AbbDQOrk (ORCPT ); Fri, 17 Apr 2015 10:47:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47600 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932774AbbDQOrd (ORCPT ); Fri, 17 Apr 2015 10:47:33 -0400 Date: Fri, 17 Apr 2015 15:06:42 +0200 From: Oleg Nesterov To: Borislav Petkov Cc: Ingo Molnar , LKML , Rik van Riel , Andy Lutomirski , "H. Peter Anvin" , Linus Torvalds , Tavis Ormandy , Thomas Gleixner Subject: Re: [PATCH -v1.1] x86/fpu: Load xsave pointer *after* initialization Message-ID: <20150417130641.GA27064@redhat.com> References: <20150416183108.GA10121@pd.tnic> <1429209697-5902-1-git-send-email-bp@alien8.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429209697-5902-1-git-send-email-bp@alien8.de> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/16, Borislav Petkov wrote: > > Box exploded with the splat at the end because we do cache > &target->thread.fpu.state->xsave into the local variable xsave but > that pointer is NULL at that time and it gets initialized later, in > init_fpu(), see > > e7f180dcd8ab ("x86/fpu: Change xstateregs_get()/set() to use > ->xsave.i387 rather than ->fxsave") Yes, my fault. Thanks a lot Borislav! Acked-by: Oleg Nesterov > Fix is simple: load xsave *after* init_fpu() has run. > > BUG: unable to handle kernel NULL pointer dereference at 00000000000001d0 > IP: [] xstateregs_get+0x7a/0x120 > PGD 7a884067 PUD 7a9aa067 PMD 0 > Oops: 0002 [#1] PREEMPT SMP > Modules linked in: > CPU: 0 PID: 4239 Comm: gdb Not tainted 4.0.0+ #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 > task: ffff880079112740 ti: ffff88007afe4000 task.ti: ffff88007afe4000 > RIP: 0010:[] [] xstateregs_get+0x7a/0x120 > RSP: 0018:ffff88007afe7dc8 EFLAGS: 00010202 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > RDX: 0000000000000007 RSI: 0000000000000000 RDI: ffff88007a0063c0 > RBP: ffff88007afe7e08 R08: 00000000001da820 R09: ffff88007a006080 > R10: ffffffff8100fde0 R11: 0000000000000001 R12: 0000000000000240 > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 > FS: 00007ffff7fdd700(0000) GS:ffff88007c200000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000000001d0 CR3: 000000007a80e000 CR4: 00000000000406f0 > Stack: > ffff88007afe7dd8 00007fffffffe160 ffff880079112740 0000000000004204 > ffff8800791113a0 ffff88007afe7e48 0000000000000202 00007fffffffe3a0 > ffff88007afe7e38 ffffffff81061505 ffff8800791113a0 00007fffffffe3a0 > Call Trace: > ptrace_regset > ptrace_request > ? wait_task_inactive > ? preempt_count_sub > arch_ptrace > ? ptrace_get_task_struct > SyS_ptrace > system_call_fastpath > [ 137.340152] Code: 41 89 c7 74 1c 44 89 f8 48 8b 5d d8 4c 8b 65 e0 4c 8b 6d e8 4c 8b 75 f0 4c 8b 7d f8 c9 c3 0f 1f 00 48 8b 15 c9 3d d2 00 45 85 e4 <49> 89 96 d0 01 00 00 48 8b 15 c0 3d d2 00 49 89 96 d8 01 00 00 > RIP [] xstateregs_get+0x7a/0x120 > RSP > CR2: 00000000000001d0 > ---[ end trace a1613adc60469c76 ]--- > > Signed-off-by: Borislav Petkov > Cc: Oleg Nesterov > Cc: Rik van Riel > Cc: Andy Lutomirski > Cc: H. Peter Anvin > Cc: Linus Torvalds > Cc: Tavis Ormandy > Cc: Thomas Gleixner > --- > arch/x86/kernel/i387.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c > index 367f39d35e9c..009183276bb7 100644 > --- a/arch/x86/kernel/i387.c > +++ b/arch/x86/kernel/i387.c > @@ -341,7 +341,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset, > unsigned int pos, unsigned int count, > void *kbuf, void __user *ubuf) > { > - struct xsave_struct *xsave = &target->thread.fpu.state->xsave; > + struct xsave_struct *xsave; > int ret; > > if (!cpu_has_xsave) > @@ -351,6 +351,8 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset, > if (ret) > return ret; > > + xsave = &target->thread.fpu.state->xsave; > + > /* > * Copy the 48bytes defined by the software first into the xstate > * memory layout in the thread struct, so that we can copy the entire > @@ -369,7 +371,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, > unsigned int pos, unsigned int count, > const void *kbuf, const void __user *ubuf) > { > - struct xsave_struct *xsave = &target->thread.fpu.state->xsave; > + struct xsave_struct *xsave; > int ret; > > if (!cpu_has_xsave) > @@ -379,6 +381,8 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, > if (ret) > return ret; > > + xsave = &target->thread.fpu.state->xsave; > + > ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1); > /* > * mxcsr reserved bits must be masked to zero for security reasons. > -- > 2.3.5 >