[PATCH] x86/fpu: Load xsave pointer *after* initialization

[PATCH] x86/fpu: Load xsave pointer *after* initialization

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

So I was playing with gdb today and did this simple thing:

gdb /bin/ls

...

(gdb) run

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")

Fix is simple: load xsave *after* init_fpu() has run.

BUG: unable to handle kernel NULL pointer dereference at 00000000000001d0
IP: [<ffffffff8100fe5a>] 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:[<ffffffff8100fe5a>]  [<ffffffff8100fe5a>] 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  [<ffffffff8100fe5a>] xstateregs_get+0x7a/0x120
 RSP <ffff88007afe7dc8>
CR2: 00000000000001d0
---[ end trace a1613adc60469c76 ]---

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Tavis Ormandy <taviso@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/i387.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 367f39d35e9c..138bd4bbbd4d 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
-- 
2.3.5

Re: [PATCH] x86/fpu: Load xsave pointer *after* initialization

[Rik van Riel]

On 04/16/2015 02:05 PM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> So I was playing with gdb today and did this simple thing:
> 
> gdb /bin/ls
> 
> ...
> 
> (gdb) run
> 
> 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

> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Tavis Ormandy <taviso@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>

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

Re: [PATCH] x86/fpu: Load xsave pointer *after* initialization

[Oleg Nesterov]

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")

OOPS! thanks a lot!!!

> --- 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;
> +

Yes, but don't we need the same change in xstateregs_set() ?

Oleg.

Re: [PATCH] x86/fpu: Load xsave pointer *after* initialization

[Borislav Petkov]

On Thu, Apr 16, 2015 at 08:20:37PM +0200, Oleg Nesterov wrote:
> Yes, but don't we need the same change in xstateregs_set() ?

Haha, yeah :-)

Sending v1.1 as a reply to this message.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH -v1.1] x86/fpu: Load xsave pointer *after* initialization

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

So I was playing with gdb today and did this simple thing:

gdb /bin/ls

...

(gdb) run

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")

Fix is simple: load xsave *after* init_fpu() has run.

BUG: unable to handle kernel NULL pointer dereference at 00000000000001d0
IP: [<ffffffff8100fe5a>] 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:[<ffffffff8100fe5a>]  [<ffffffff8100fe5a>] 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  [<ffffffff8100fe5a>] xstateregs_get+0x7a/0x120
 RSP <ffff88007afe7dc8>
CR2: 00000000000001d0
---[ end trace a1613adc60469c76 ]---

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Tavis Ormandy <taviso@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 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

Re: [PATCH -v1.1] x86/fpu: Load xsave pointer *after* initialization

[Oleg Nesterov]

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 <oleg@redhat.com>

> Fix is simple: load xsave *after* init_fpu() has run.
> 
> BUG: unable to handle kernel NULL pointer dereference at 00000000000001d0
> IP: [<ffffffff8100fe5a>] 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:[<ffffffff8100fe5a>]  [<ffffffff8100fe5a>] 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  [<ffffffff8100fe5a>] xstateregs_get+0x7a/0x120
>  RSP <ffff88007afe7dc8>
> CR2: 00000000000001d0
> ---[ end trace a1613adc60469c76 ]---
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Tavis Ormandy <taviso@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  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
> 

[tip:x86/urgent] x86/fpu: Load xsave pointer *after* initialization

[tip-bot for Borislav Petkov]

Commit-ID:  18ecb3bfa5a9f6fffbb3eeb4369f0b9463438ec0
Gitweb:     http://git.kernel.org/tip/18ecb3bfa5a9f6fffbb3eeb4369f0b9463438ec0
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Thu, 16 Apr 2015 20:41:37 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 17 Apr 2015 10:15:47 +0200

x86/fpu: Load xsave pointer *after* initialization

So I was playing with gdb today and did this simple thing:

	gdb /bin/ls

	...

	(gdb) run

Box exploded with this splat:

	BUG: unable to handle kernel NULL pointer dereference at 00000000000001d0
	IP: [<ffffffff8100fe5a>] xstateregs_get+0x7a/0x120
	[...]

	Call Trace:
	 ptrace_regset
	 ptrace_request
	 ? wait_task_inactive
	 ? preempt_count_sub
	 arch_ptrace
	 ? ptrace_get_task_struct
	 SyS_ptrace
	 system_call_fastpath

... 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")

The fix is simple: load xsave *after* init_fpu() has run.

Also do the same in xstateregs_set(), as suggested by Oleg Nesterov.

Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Tavis Ormandy <taviso@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1429209697-5902-1-git-send-email-bp@alien8.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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 367f39d..00918327 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.