All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
@ 2018-02-14  2:27 Josh Poimboeuf
  2018-02-14  4:19 ` Dave Hansen
  2018-02-14  7:35 ` [PATCH] x86/entry/64: Fix CR3 restore order " Ingo Molnar
  0 siblings, 2 replies; 10+ messages in thread
From: Josh Poimboeuf @ 2018-02-14  2:27 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
	David Woodhouse, Thomas Gleixner, Ingo Molnar

The paranoid exit code only restores the saved CR3 when it switches back
to the user GS.  However, even in the kernel GS case, it's possible that
it needs to restore a user CR3, if for example, the paranoid exception
occurred in the syscall exit path between SWITCH_TO_USER_CR3_STACK and
SWAPGS.

Fix that issue by making the CR3 restore unconditional.  This is
symmetrical with the unconditional CR3 save in paranoid_entry().

Also, since RESTORE_CR3 is now done before the EBX compare, it needs to
use a different scratch register (R15 instead of RBX).

I haven't actually seen any real-world bugs caused by this, so I'm not
sure how theoretical it is.  I just stumbled upon it in code review when
looking for another bug.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/entry/entry_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index cd216c9431e1..68c95a09b48d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
 	UNWIND_HINT_REGS
 	DISABLE_INTERRUPTS(CLBR_ANY)
 	TRACE_IRQS_OFF_DEBUG
+	RESTORE_CR3	scratch_reg=%r15 save_reg=%r14
 	testl	%ebx, %ebx			/* swapgs needed? */
 	jnz	.Lparanoid_exit_no_swapgs
 	TRACE_IRQS_IRETQ
-	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
 	SWAPGS_UNSAFE_STACK
 	jmp	.Lparanoid_exit_restore
 .Lparanoid_exit_no_swapgs:
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  2:27 [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit() Josh Poimboeuf
@ 2018-02-14  4:19 ` Dave Hansen
  2018-02-14  7:39   ` Ingo Molnar
  2018-02-14  7:35 ` [PATCH] x86/entry/64: Fix CR3 restore order " Ingo Molnar
  1 sibling, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2018-02-14  4:19 UTC (permalink / raw)
  To: Josh Poimboeuf, x86
  Cc: linux-kernel, Andy Lutomirski, Peter Zijlstra, David Woodhouse,
	Thomas Gleixner, Ingo Molnar

On 02/13/2018 06:27 PM, Josh Poimboeuf wrote:
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
>  	UNWIND_HINT_REGS
>  	DISABLE_INTERRUPTS(CLBR_ANY)
>  	TRACE_IRQS_OFF_DEBUG
> +	RESTORE_CR3	scratch_reg=%r15 save_reg=%r14
>  	testl	%ebx, %ebx			/* swapgs needed? */
>  	jnz	.Lparanoid_exit_no_swapgs
>  	TRACE_IRQS_IRETQ
> -	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
>  	SWAPGS_UNSAFE_STACK
>  	jmp	.Lparanoid_exit_restore
>  .Lparanoid_exit_no_swapgs:

TRACE_IRQS_* call non-entry functions that are not mapped by the user
CR3.  How can this possibly work?  What am I missing?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  2:27 [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit() Josh Poimboeuf
  2018-02-14  4:19 ` Dave Hansen
@ 2018-02-14  7:35 ` Ingo Molnar
  1 sibling, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2018-02-14  7:35 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: x86, linux-kernel, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
	David Woodhouse, Thomas Gleixner


* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> I haven't actually seen any real-world bugs caused by this, so I'm not
> sure how theoretical it is.  I just stumbled upon it in code review when
> looking for another bug.

I believe it's a real bug, but the fix is wrong with irq tracing or lockdep 
enabled as Dave points out.

I think the reason we haven't seen this bug yet is that "paranoid" entry points 
are limited to:

idtentry double_fault			do_double_fault			has_error_code=1 paranoid=2
idtentry debug			do_debug		has_error_code=0	paranoid=1 shift_ist=DEBUG_STACK
idtentry int3			do_int3			has_error_code=0	paranoid=1 shift_ist=DEBUG_STACK
idtentry machine_check		do_mce			has_error_code=0	paranoid=1

Only machine_check is one that will interrupt an IRQS-off critical section 
asynchronously - and machine check events are rare.

The other main asynchronous entries are NMI entries, which can be very high-freq 
with perf profiling, but they are special: they don't use the 'idtentry' macro but 
are open coded and restore user CR3 unconditionally so don't seem to have this 
bug.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  4:19 ` Dave Hansen
@ 2018-02-14  7:39   ` Ingo Molnar
  2018-02-14 16:11     ` Josh Poimboeuf
                       ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Ingo Molnar @ 2018-02-14  7:39 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Josh Poimboeuf, x86, linux-kernel, Andy Lutomirski,
	Peter Zijlstra, David Woodhouse, Thomas Gleixner, Linus Torvalds,
	Peter Zijlstra


* Dave Hansen <dave.hansen@intel.com> wrote:

> On 02/13/2018 06:27 PM, Josh Poimboeuf wrote:
> > --- a/arch/x86/entry/entry_64.S
> > +++ b/arch/x86/entry/entry_64.S
> > @@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
> >  	UNWIND_HINT_REGS
> >  	DISABLE_INTERRUPTS(CLBR_ANY)
> >  	TRACE_IRQS_OFF_DEBUG
> > +	RESTORE_CR3	scratch_reg=%r15 save_reg=%r14
> >  	testl	%ebx, %ebx			/* swapgs needed? */
> >  	jnz	.Lparanoid_exit_no_swapgs
> >  	TRACE_IRQS_IRETQ
> > -	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
> >  	SWAPGS_UNSAFE_STACK
> >  	jmp	.Lparanoid_exit_restore
> >  .Lparanoid_exit_no_swapgs:
> 
> TRACE_IRQS_* call non-entry functions that are not mapped by the user
> CR3.  How can this possibly work?  What am I missing?

How about something like the patch below? (Totally untested)

Thanks,

	Ingo
---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index cd216c9431e1..8971bd64d515 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1175,6 +1175,7 @@ ENTRY(paranoid_exit)
 	jmp	.Lparanoid_exit_restore
 .Lparanoid_exit_no_swapgs:
 	TRACE_IRQS_IRETQ_DEBUG
+	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
 .Lparanoid_exit_restore:
 	jmp restore_regs_and_return_to_kernel
 END(paranoid_exit)

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  7:39   ` Ingo Molnar
@ 2018-02-14 16:11     ` Josh Poimboeuf
  2018-02-14 22:27       ` Ingo Molnar
  2018-02-14 16:13     ` Thomas Gleixner
                       ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2018-02-14 16:11 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Dave Hansen, x86, linux-kernel, Andy Lutomirski, Peter Zijlstra,
	David Woodhouse, Thomas Gleixner, Linus Torvalds, Peter Zijlstra

On Wed, Feb 14, 2018 at 08:39:11AM +0100, Ingo Molnar wrote:
> 
> * Dave Hansen <dave.hansen@intel.com> wrote:
> 
> > On 02/13/2018 06:27 PM, Josh Poimboeuf wrote:
> > > --- a/arch/x86/entry/entry_64.S
> > > +++ b/arch/x86/entry/entry_64.S
> > > @@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
> > >  	UNWIND_HINT_REGS
> > >  	DISABLE_INTERRUPTS(CLBR_ANY)
> > >  	TRACE_IRQS_OFF_DEBUG
> > > +	RESTORE_CR3	scratch_reg=%r15 save_reg=%r14
> > >  	testl	%ebx, %ebx			/* swapgs needed? */
> > >  	jnz	.Lparanoid_exit_no_swapgs
> > >  	TRACE_IRQS_IRETQ
> > > -	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
> > >  	SWAPGS_UNSAFE_STACK
> > >  	jmp	.Lparanoid_exit_restore
> > >  .Lparanoid_exit_no_swapgs:
> > 
> > TRACE_IRQS_* call non-entry functions that are not mapped by the user
> > CR3.  How can this possibly work?  What am I missing?
> 
> How about something like the patch below? (Totally untested)
> 
> Thanks,
> 
> 	Ingo
> ---
>  arch/x86/entry/entry_64.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cd216c9431e1..8971bd64d515 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1175,6 +1175,7 @@ ENTRY(paranoid_exit)
>  	jmp	.Lparanoid_exit_restore
>  .Lparanoid_exit_no_swapgs:
>  	TRACE_IRQS_IRETQ_DEBUG
> +	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
>  .Lparanoid_exit_restore:
>  	jmp restore_regs_and_return_to_kernel
>  END(paranoid_exit)

Dave was right, my patch was obviously bogus.  I couldn't figure out a
real reproducer, so I made an artificial one (see below) and can confirm
that your patch fixes it.

I would resubmit the patch, but now you're the author, so I'm not sure
how that works with the SOB.

Feel free to add my

  Reported-and-tested-by: Josh Poimboeuf <jpoimboe@redhat.com>

Thanks!

-------------

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 68c95a09b48d..c181eb23109b 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -325,6 +325,8 @@ syscall_return_via_sysret:
 	 */
 	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
 
+	int3
+
 	popq	%rdi
 	popq	%rsp
 	USERGS_SYSRET64
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 3d9b2308e7fa..74fabcdf6c36 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -605,6 +605,8 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 		goto exit;
 #endif
 
+	goto exit;
+
 	if (notify_die(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,
 			SIGTRAP) == NOTIFY_STOP)
 		goto exit;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  7:39   ` Ingo Molnar
  2018-02-14 16:11     ` Josh Poimboeuf
@ 2018-02-14 16:13     ` Thomas Gleixner
  2018-02-14 17:16     ` Andy Lutomirski
                       ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2018-02-14 16:13 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Dave Hansen, Josh Poimboeuf, x86, linux-kernel, Andy Lutomirski,
	Peter Zijlstra, David Woodhouse, Linus Torvalds, Peter Zijlstra

On Wed, 14 Feb 2018, Ingo Molnar wrote:
> * Dave Hansen <dave.hansen@intel.com> wrote:
> 
> > On 02/13/2018 06:27 PM, Josh Poimboeuf wrote:
> > > --- a/arch/x86/entry/entry_64.S
> > > +++ b/arch/x86/entry/entry_64.S
> > > @@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
> > >  	UNWIND_HINT_REGS
> > >  	DISABLE_INTERRUPTS(CLBR_ANY)
> > >  	TRACE_IRQS_OFF_DEBUG
> > > +	RESTORE_CR3	scratch_reg=%r15 save_reg=%r14
> > >  	testl	%ebx, %ebx			/* swapgs needed? */
> > >  	jnz	.Lparanoid_exit_no_swapgs
> > >  	TRACE_IRQS_IRETQ
> > > -	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
> > >  	SWAPGS_UNSAFE_STACK
> > >  	jmp	.Lparanoid_exit_restore
> > >  .Lparanoid_exit_no_swapgs:
> > 
> > TRACE_IRQS_* call non-entry functions that are not mapped by the user
> > CR3.  How can this possibly work?  What am I missing?
> 
> How about something like the patch below? (Totally untested)

But correct ....


> Thanks,
> 
> 	Ingo
> ---
>  arch/x86/entry/entry_64.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cd216c9431e1..8971bd64d515 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1175,6 +1175,7 @@ ENTRY(paranoid_exit)
>  	jmp	.Lparanoid_exit_restore
>  .Lparanoid_exit_no_swapgs:
>  	TRACE_IRQS_IRETQ_DEBUG
> +	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
>  .Lparanoid_exit_restore:
>  	jmp restore_regs_and_return_to_kernel
>  END(paranoid_exit)
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14  7:39   ` Ingo Molnar
  2018-02-14 16:11     ` Josh Poimboeuf
  2018-02-14 16:13     ` Thomas Gleixner
@ 2018-02-14 17:16     ` Andy Lutomirski
  2018-02-14 23:31     ` [tip:x86/pti] x86/entry/64: Fix CR3 restore " tip-bot for Ingo Molnar
  2018-02-15  0:31     ` tip-bot for Ingo Molnar
  4 siblings, 0 replies; 10+ messages in thread
From: Andy Lutomirski @ 2018-02-14 17:16 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Dave Hansen, Josh Poimboeuf, X86 ML, LKML, Andy Lutomirski,
	Peter Zijlstra, David Woodhouse, Thomas Gleixner, Linus Torvalds,
	Peter Zijlstra

On Wed, Feb 14, 2018 at 7:39 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dave Hansen <dave.hansen@intel.com> wrote:
>
>> On 02/13/2018 06:27 PM, Josh Poimboeuf wrote:
>> > --- a/arch/x86/entry/entry_64.S
>> > +++ b/arch/x86/entry/entry_64.S
>> > @@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
>> >     UNWIND_HINT_REGS
>> >     DISABLE_INTERRUPTS(CLBR_ANY)
>> >     TRACE_IRQS_OFF_DEBUG
>> > +   RESTORE_CR3     scratch_reg=%r15 save_reg=%r14
>> >     testl   %ebx, %ebx                      /* swapgs needed? */
>> >     jnz     .Lparanoid_exit_no_swapgs
>> >     TRACE_IRQS_IRETQ
>> > -   RESTORE_CR3     scratch_reg=%rbx save_reg=%r14
>> >     SWAPGS_UNSAFE_STACK
>> >     jmp     .Lparanoid_exit_restore
>> >  .Lparanoid_exit_no_swapgs:
>>
>> TRACE_IRQS_* call non-entry functions that are not mapped by the user
>> CR3.  How can this possibly work?  What am I missing?
>
> How about something like the patch below? (Totally untested)
>
> Thanks,
>
>         Ingo
> ---
>  arch/x86/entry/entry_64.S | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cd216c9431e1..8971bd64d515 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1175,6 +1175,7 @@ ENTRY(paranoid_exit)
>         jmp     .Lparanoid_exit_restore
>  .Lparanoid_exit_no_swapgs:
>         TRACE_IRQS_IRETQ_DEBUG
> +       RESTORE_CR3     scratch_reg=%rbx save_reg=%r14
>  .Lparanoid_exit_restore:
>         jmp restore_regs_and_return_to_kernel
>  END(paranoid_exit)

Reviewed-by: Andy Lutomirski <luto@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()
  2018-02-14 16:11     ` Josh Poimboeuf
@ 2018-02-14 22:27       ` Ingo Molnar
  0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2018-02-14 22:27 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Dave Hansen, x86, linux-kernel, Andy Lutomirski, Peter Zijlstra,
	David Woodhouse, Thomas Gleixner, Linus Torvalds, Peter Zijlstra


* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Dave was right, my patch was obviously bogus.  I couldn't figure out a
> real reproducer, so I made an artificial one (see below) and can confirm
> that your patch fixes it.
> 
> I would resubmit the patch, but now you're the author, so I'm not sure
> how that works with the SOB.

I'd just have made you the author of the oneliner fix, in fair payment for writing 
the changelog! ;-)

For anything larger and more complex we can do:

  Signed-off-by: Ingo Molnar <mingo@kernel.org>
  Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
  Signed-off-by: Ingo Molnar <mingo@kernel.org>

Which is technically an unusual but still correct 'SoB chain' describing the 
true route of the patch.

See for example these scheduler commits:

  8c0944cee7af: sched/deadline: Rename __dl_clear() to __dl_sub()
  07f06cb3b5f6: sched: Start stopper early
  7675104990ed: sched: Implement lockless wake-queues

In particular 7675104990ed is a good example:

  Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
  [tweaks, adjustments, comments, etc.]
  Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
  Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
  Acked-by: Thomas Gleixner <tglx@linutronix.de>
  Signed-off-by: Ingo Molnar <mingo@kernel.org>

With his developer hat on PeterZ sent out a first implementation which Davidlohr 
fixed, tested and signed off on - which PeterZ then applied with his maintainer 
hat on, which Thomas acked and I committed to the scheduler git tree.

It would require two commits (one of which could easily be a bisection-breaker) to 
express this in any other way.

But such chains are pretty rare and very much the exception: usually either the 
original author takes care of everything, or the secondary author makes so many 
changes that the secondary author becomes the primary author and the original 
author is credited via a copyright notice and/or a tag like:

  Originally-From: Ingo Molnar <mingo@kernel.org>

> Feel free to add my
> 
>   Reported-and-tested-by: Josh Poimboeuf <jpoimboe@redhat.com>

Thanks and added!

BTW:

> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -325,6 +325,8 @@ syscall_return_via_sysret:
>  	 */
>  	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
>  
> +	int3
> +
>  	popq	%rdi
>  	popq	%rsp
>  	USERGS_SYSRET64
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 3d9b2308e7fa..74fabcdf6c36 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -605,6 +605,8 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
>  		goto exit;
>  #endif
>  
> +	goto exit;

That's a very clever way to test such races!

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip:x86/pti] x86/entry/64: Fix CR3 restore in paranoid_exit()
  2018-02-14  7:39   ` Ingo Molnar
                       ` (2 preceding siblings ...)
  2018-02-14 17:16     ` Andy Lutomirski
@ 2018-02-14 23:31     ` tip-bot for Ingo Molnar
  2018-02-15  0:31     ` tip-bot for Ingo Molnar
  4 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Ingo Molnar @ 2018-02-14 23:31 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: peterz, linux-kernel, torvalds, mingo, dave.hansen, arjan,
	jpoimboe, bp, hpa, gregkh, luto, dwmw2, dan.j.williams, tglx

Commit-ID:  48753793350974b7afe9598fd1dc46b2f1f47c2d
Gitweb:     https://git.kernel.org/tip/48753793350974b7afe9598fd1dc46b2f1f47c2d
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Wed, 14 Feb 2018 08:39:11 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 15 Feb 2018 00:28:03 +0100

x86/entry/64: Fix CR3 restore in paranoid_exit()

Josh Poimboeuf noticed the following bug:

 "The paranoid exit code only restores the saved CR3 when it switches back
  to the user GS.  However, even in the kernel GS case, it's possible that
  it needs to restore a user CR3, if for example, the paranoid exception
  occurred in the syscall exit path between SWITCH_TO_USER_CR3_STACK and
  SWAPGS."

Josh also confirmed via targeted testing that it's possible to hit this bug.

Fix the bug by also restoring CR3 in the paranoid_exit_no_swapgs branch.

The reason we haven't seen this bug reported by users yet is probably because
"paranoid" entry points are limited to the following cases:

 idtentry double_fault       do_double_fault  has_error_code=1  paranoid=2
 idtentry debug              do_debug         has_error_code=0  paranoid=1 shift_ist=DEBUG_STACK
 idtentry int3               do_int3          has_error_code=0  paranoid=1 shift_ist=DEBUG_STACK
 idtentry machine_check      do_mce           has_error_code=0  paranoid=1

Amongst those entry points only machine_check is one that will interrupt an
IRQS-off critical section asynchronously - and machine check events are rare.

The other main asynchronous entries are NMI entries, which can be very high-freq
with perf profiling, but they are special: they don't use the 'idtentry' macro but
are open coded and restore user CR3 unconditionally so don't have this bug.

Reported-and-tested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180214073910.boevmg65upbk3vqb@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1c5420420..4fd9044 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1168,6 +1168,7 @@ ENTRY(paranoid_exit)
 	jmp	.Lparanoid_exit_restore
 .Lparanoid_exit_no_swapgs:
 	TRACE_IRQS_IRETQ_DEBUG
+	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
 .Lparanoid_exit_restore:
 	jmp restore_regs_and_return_to_kernel
 END(paranoid_exit)

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [tip:x86/pti] x86/entry/64: Fix CR3 restore in paranoid_exit()
  2018-02-14  7:39   ` Ingo Molnar
                       ` (3 preceding siblings ...)
  2018-02-14 23:31     ` [tip:x86/pti] x86/entry/64: Fix CR3 restore " tip-bot for Ingo Molnar
@ 2018-02-15  0:31     ` tip-bot for Ingo Molnar
  4 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Ingo Molnar @ 2018-02-15  0:31 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: dan.j.williams, linux-kernel, bp, luto, dwmw2, dave.hansen,
	jpoimboe, tglx, mingo, torvalds, gregkh, hpa, peterz, arjan

Commit-ID:  e48657573481a5dff7cfdc3d57005c80aa816500
Gitweb:     https://git.kernel.org/tip/e48657573481a5dff7cfdc3d57005c80aa816500
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Wed, 14 Feb 2018 08:39:11 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 15 Feb 2018 01:15:54 +0100

x86/entry/64: Fix CR3 restore in paranoid_exit()

Josh Poimboeuf noticed the following bug:

 "The paranoid exit code only restores the saved CR3 when it switches back
  to the user GS.  However, even in the kernel GS case, it's possible that
  it needs to restore a user CR3, if for example, the paranoid exception
  occurred in the syscall exit path between SWITCH_TO_USER_CR3_STACK and
  SWAPGS."

Josh also confirmed via targeted testing that it's possible to hit this bug.

Fix the bug by also restoring CR3 in the paranoid_exit_no_swapgs branch.

The reason we haven't seen this bug reported by users yet is probably because
"paranoid" entry points are limited to the following cases:

 idtentry double_fault       do_double_fault  has_error_code=1  paranoid=2
 idtentry debug              do_debug         has_error_code=0  paranoid=1 shift_ist=DEBUG_STACK
 idtentry int3               do_int3          has_error_code=0  paranoid=1 shift_ist=DEBUG_STACK
 idtentry machine_check      do_mce           has_error_code=0  paranoid=1

Amongst those entry points only machine_check is one that will interrupt an
IRQS-off critical section asynchronously - and machine check events are rare.

The other main asynchronous entries are NMI entries, which can be very high-freq
with perf profiling, but they are special: they don't use the 'idtentry' macro but
are open coded and restore user CR3 unconditionally so don't have this bug.

Reported-and-tested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180214073910.boevmg65upbk3vqb@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1c5420420..4fd9044 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1168,6 +1168,7 @@ ENTRY(paranoid_exit)
 	jmp	.Lparanoid_exit_restore
 .Lparanoid_exit_no_swapgs:
 	TRACE_IRQS_IRETQ_DEBUG
+	RESTORE_CR3	scratch_reg=%rbx save_reg=%r14
 .Lparanoid_exit_restore:
 	jmp restore_regs_and_return_to_kernel
 END(paranoid_exit)

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-02-15  0:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14  2:27 [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit() Josh Poimboeuf
2018-02-14  4:19 ` Dave Hansen
2018-02-14  7:39   ` Ingo Molnar
2018-02-14 16:11     ` Josh Poimboeuf
2018-02-14 22:27       ` Ingo Molnar
2018-02-14 16:13     ` Thomas Gleixner
2018-02-14 17:16     ` Andy Lutomirski
2018-02-14 23:31     ` [tip:x86/pti] x86/entry/64: Fix CR3 restore " tip-bot for Ingo Molnar
2018-02-15  0:31     ` tip-bot for Ingo Molnar
2018-02-14  7:35 ` [PATCH] x86/entry/64: Fix CR3 restore order " Ingo Molnar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.