Skip to contentSkip to conversation

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

PATCH In review Josh Poimboeuf 6 participants 10 messages Feb 14, 2018 – Feb 15, 2018

Subsystems (2)
x86 x86 entry code
lore
Share & tools
lore

Josh Poimboeuf

author
#0 lore raw

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

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 <[email protected]>
---
 arch/x86/entry/entry_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/entry/entry_64.S +1 -1
index cd216c9431e1..68c95a09b48d 100644
@@ -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:
Signature · 1 line
2.14.3
#1 lore raw
Quoted 4 lines · Josh Poimboeuf
arch/x86/entry/entry_64.S +0 -0
@@ -1167,10 +1167,10 @@ ENTRY(paranoid_exit)
 	UNWIND_HINT_REGS

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1176

Quoted 10 lines
arch/x86/entry/entry_64.S +1 -1
 	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?

#2 lore raw
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Josh Poimboeuf
arch/x86/entry/entry_64.S +1 -0
@@ -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
Quoted 8 lines

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1176

Quoted 5 lines
arch/x86/entry/entry_64.S +0 -1
 	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(+)
arch/x86/entry/entry_64.S +1 -0
index cd216c9431e1..8971bd64d515 100644
@@ -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)
author
#3 lore raw
Quoted 36 lines · Ingo Molnar
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Josh Poimboeuf
arch/x86/entry/entry_64.S +1 -0
@@ -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
Quoted 8 lines

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1176

Quoted 5 lines
arch/x86/entry/entry_64.S +0 -1
 	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(+)
arch/x86/entry/entry_64.S +0 -0
index cd216c9431e1..8971bd64d515 100644

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1181

Quoted 7 lines
arch/x86/entry/entry_64.S +1 -0
@@ -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 <[email protected]>

Thanks!

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

arch/x86/entry/entry_64.S +2 -0
index 68c95a09b48d..c181eb23109b 100644
@@ -325,6 +325,8 @@ syscall_return_via_sysret:
 	 */
 	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
 
+	int3
+
 	popq	%rdi
 	popq	%rsp
 	USERGS_SYSRET64
arch/x86/kernel/traps.c +2 -0
index 3d9b2308e7fa..74fabcdf6c36 100644
@@ -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;
Acked-by Thomas Gleixner
#4 lore raw
Quoted 6 lines · Josh Poimboeuf

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 <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>

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) <[email protected]>
[tweaks, adjustments, comments, etc.]
Signed-off-by Davidlohr Bueso <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Acked-by Thomas Gleixner <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>

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 <[email protected]>

Quoted 3 lines

Feel free to add my

Reported-and-tested-by Josh Poimboeuf <[email protected]>

Thanks and added!

BTW:

Quoted 16 lines
arch/x86/entry/entry_64.S +2 -0
@@ -325,6 +325,8 @@ syscall_return_via_sysret:
 	 */
 	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
 
+	int3
+
 	popq	%rdi
 	popq	%rsp
 	USERGS_SYSRET64
arch/x86/kernel/traps.c +0 -0
index 3d9b2308e7fa..74fabcdf6c36 100644

arch/x86/kernel/traps.c dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code) · line 608

Quoted 4 lines
arch/x86/kernel/traps.c +1 -0
@@ -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

#5 lore raw
In reply to Ingo Molnar
Quoted 21 lines · Ingo Molnar
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Josh Poimboeuf
arch/x86/entry/entry_64.S +1 -0
@@ -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
Quoted 8 lines

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1176

Quoted 5 lines
arch/x86/entry/entry_64.S +0 -1
 	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?

Quoted 1 line

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

But correct ....

Quoted 20 lines

Thanks,

Ingo

---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)
arch/x86/entry/entry_64.S +1 -0
index cd216c9431e1..8971bd64d515 100644
@@ -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)
maintainer Reviewed-by
#6 lore raw
In reply to Ingo Molnar
Quoted 36 lines · Ingo Molnar
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Josh Poimboeuf
arch/x86/entry/entry_64.S +1 -0
@@ -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
Quoted 8 lines

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1176

Quoted 5 lines
arch/x86/entry/entry_64.S +0 -1
    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(+)
arch/x86/entry/entry_64.S +0 -0
index cd216c9431e1..8971bd64d515 100644

arch/x86/entry/entry_64.S ENTRY(paranoid_exit) · line 1181

Quoted 7 lines
arch/x86/entry/entry_64.S +1 -0
@@ -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 <[email protected]>
merge bot
#7 lore raw

[tip:x86/pti] x86/entry/64: Fix CR3 restore in paranoid_exit()

In reply to Ingo Molnar
x86/entry/64: Fix CR3 restore in paranoid_exit()
Commit-ID:  48753793350974b7afe9598fd1dc46b2f1f47c2d
Gitweb:     https://git.kernel.org/tip/48753793350974b7afe9598fd1dc46b2f1f47c2d
Author:     Ingo Molnar <[email protected]>
AuthorDate: Wed, 14 Feb 2018 08:39:11 +0100
Committer:  Ingo Molnar <[email protected]>
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 <[email protected]>
Reviewed-by Andy Lutomirski <[email protected]>
Acked-by Thomas Gleixner <[email protected]>
Cc Arjan van de Ven <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dan Williams <[email protected]>
Cc Dave Hansen <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)
arch/x86/entry/entry_64.S +1 -0
index 1c5420420..4fd9044 100644
@@ -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)
merge bot
#8 lore raw

[tip:x86/pti] x86/entry/64: Fix CR3 restore in paranoid_exit()

In reply to Ingo Molnar
x86/entry/64: Fix CR3 restore in paranoid_exit()
Commit-ID:  e48657573481a5dff7cfdc3d57005c80aa816500
Gitweb:     https://git.kernel.org/tip/e48657573481a5dff7cfdc3d57005c80aa816500
Author:     Ingo Molnar <[email protected]>
AuthorDate: Wed, 14 Feb 2018 08:39:11 +0100
Committer:  Ingo Molnar <[email protected]>
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 <[email protected]>
Reviewed-by Andy Lutomirski <[email protected]>
Acked-by Thomas Gleixner <[email protected]>
Cc Arjan van de Ven <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Dan Williams <[email protected]>
Cc Dave Hansen <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/entry/entry_64.S | 1 +
 1 file changed, 1 insertion(+)
arch/x86/entry/entry_64.S +1 -0
index 1c5420420..4fd9044 100644
@@ -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)
#9 lore raw
In reply to Josh Poimboeuf
Quoted 3 lines · Josh Poimboeuf

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