x86: Various SYSENTER/SYSEXIT/#DB fixes and cleanups

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

That's all we need to do.

Don't get too excited -- our handling is still buggy on 32-bit kernels. There's nothing wrong with the SYSENTER code itself, but the #DB prologue has a clever fixup for traps on the very first instruction of entry_SYSENTER_32, and the fixup doesn't work quite correctly. The next two patches will fix that.

[1] We could probably prevent it by forcing BTF on at all times and making sure we clear TF before any branches in the SYSENTER code. Needless to say, this is a bad idea.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S        | 42 ++++++++++++++++++++++----------
 arch/x86/entry/entry_64_compat.S |  9 ++++++-
 arch/x86/include/asm/proto.h     | 15 ++++++++++--
 arch/x86/kernel/traps.c          | 52 +++++++++++++++++++++++++++++++++-------
 4 files changed, 94 insertions(+), 24 deletions(-)

arch/x86/entry/entry_32.S +13 -1

index a8c3424c3392..7700cf695d8c 100644
@@ -287,7 +287,26 @@ need_resched:
 END(resume_kernel)
 #endif
 
-	# SYSENTER  call handler stub
+GLOBAL(__begin_SYSENTER_singlestep_region)
+/*
+ * All code from here through __end_SYSENTER_singlestep_region is subject
+ * to being single-stepped if a user program sets TF and executes SYSENTER.
+ * There is absolutely nothing that we can do to prevent this from happening
+ * (thanks Intel!).  To keep our handling of this situation as simple as
+ * possible, we handle TF just like AC and NT, except that our #DB handler
+ * will ignore all of the single-step traps generated in this range.
+ */
+
+#ifdef CONFIG_XEN
+/*
+ * Xen doesn't set %esp to be precisely what the normal SYSENTER

From a quick scan, kallsyms can give you the symbol size too so that you

Quoted 54 lines · Andy Lutomirski

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

That's all we need to do.

Don't get too excited -- our handling is still buggy on 32-bit kernels. There's nothing wrong with the SYSENTER code itself, but the #DB prologue has a clever fixup for traps on the very first instruction of entry_SYSENTER_32, and the fixup doesn't work quite correctly. The next two patches will fix that.

[1] We could probably prevent it by forcing BTF on at all times and making sure we clear TF before any branches in the SYSENTER code. Needless to say, this is a bad idea.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S        | 42 ++++++++++++++++++++++----------
 arch/x86/entry/entry_64_compat.S |  9 ++++++-
 arch/x86/include/asm/proto.h     | 15 ++++++++++--
 arch/x86/kernel/traps.c          | 52 +++++++++++++++++++++++++++++++++-------
 4 files changed, 94 insertions(+), 24 deletions(-)

arch/x86/entry/entry_32.S +13 -1

index a8c3424c3392..7700cf695d8c 100644
@@ -287,7 +287,26 @@ need_resched:
 END(resume_kernel)
 #endif
 
-     # SYSENTER  call handler stub
+GLOBAL(__begin_SYSENTER_singlestep_region)
+/*
+ * All code from here through __end_SYSENTER_singlestep_region is subject
+ * to being single-stepped if a user program sets TF and executes SYSENTER.
+ * There is absolutely nothing that we can do to prevent this from happening
+ * (thanks Intel!).  To keep our handling of this situation as simple as
+ * possible, we handle TF just like AC and NT, except that our #DB handler
+ * will ignore all of the single-step traps generated in this range.
+ */
+
+#ifdef CONFIG_XEN
+/*
+ * Xen doesn't set %esp to be precisely what the normal SYSENTER

arch/x86/entry/entry_32.S need_resched: · line 312

Quoted 10 lines

arch/x86/entry/entry_32.S +7 -0

+ * entry point expects, so fix it up before using the normal path.
+ */
+ENTRY(xen_sysenter_target)
+     addl    $5*4, %esp                      /* remove xen-provided frame */
+     jmp     sysenter_past_esp
+#endif
+
 ENTRY(entry_SYSENTER_32)
      movl    TSS_sysenter_sp0(%esp), %esp
 sysenter_past_esp:

Can you do this below (ontop of your diff) and get rid of those __{begin,end}_SYSENTER_singlestep_region and __end_entry_SYSENTER_compat globals and use the entry_SYSENTER_{32|compat} symbols instead?

From a quick scan, kallsyms can give you the symbol size too so that you can compute where it ends:

readelf -a vmlinux | grep entry_SYSENTER

 55454: ffffffff8170aff0    99 FUNC    GLOBAL DEFAULT    1 entry_SYSENTER_compat
 62596: ffffffff8170b053     0 NOTYPE  GLOBAL DEFAULT    1 __end_entry_SYSENTER_comp

0xffffffff8170aff0 + 99 = 0xffffffff8170b053

---
Index: b/arch/x86/entry/entry_32.S
===================================================================

arch/x86/entry/entry_32.S 2016-03-06 17:46:52.235733410 +0100 renamed from arch/x86/entry/entry_32.S 2016-03-06 17:47:10.059733163 +0100 +1 -5

@@ -297,18 +297,13 @@ GLOBAL(__begin_SYSENTER_singlestep_regio
  * will ignore all of the single-step traps generated in this range.
  */
 
+ENTRY(entry_SYSENTER_32)
 #ifdef CONFIG_XEN
-/*
- * Xen doesn't set %esp to be precisely what the normal SYSENTER
- * entry point expects, so fix it up before using the normal path.
- */
-ENTRY(xen_sysenter_target)
        addl    $5*4, %esp                      /* remove xen-provided frame */

Quoted 96 lines · Borislav Petkov

Quoted 54 lines · Andy Lutomirski

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

That's all we need to do.

Don't get too excited -- our handling is still buggy on 32-bit kernels. There's nothing wrong with the SYSENTER code itself, but the #DB prologue has a clever fixup for traps on the very first instruction of entry_SYSENTER_32, and the fixup doesn't work quite correctly. The next two patches will fix that.

[1] We could probably prevent it by forcing BTF on at all times and making sure we clear TF before any branches in the SYSENTER code. Needless to say, this is a bad idea.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S        | 42 ++++++++++++++++++++++----------
 arch/x86/entry/entry_64_compat.S |  9 ++++++-
 arch/x86/include/asm/proto.h     | 15 ++++++++++--
 arch/x86/kernel/traps.c          | 52 +++++++++++++++++++++++++++++++++-------
 4 files changed, 94 insertions(+), 24 deletions(-)

arch/x86/entry/entry_32.S +13 -1

index a8c3424c3392..7700cf695d8c 100644
@@ -287,7 +287,26 @@ need_resched:
 END(resume_kernel)
 #endif
 
-     # SYSENTER  call handler stub
+GLOBAL(__begin_SYSENTER_singlestep_region)
+/*
+ * All code from here through __end_SYSENTER_singlestep_region is subject
+ * to being single-stepped if a user program sets TF and executes SYSENTER.
+ * There is absolutely nothing that we can do to prevent this from happening
+ * (thanks Intel!).  To keep our handling of this situation as simple as
+ * possible, we handle TF just like AC and NT, except that our #DB handler
+ * will ignore all of the single-step traps generated in this range.
+ */
+
+#ifdef CONFIG_XEN
+/*
+ * Xen doesn't set %esp to be precisely what the normal SYSENTER

arch/x86/entry/entry_32.S need_resched: · line 312

Quoted 10 lines

arch/x86/entry/entry_32.S +7 -0

+ * entry point expects, so fix it up before using the normal path.
+ */
+ENTRY(xen_sysenter_target)
+     addl    $5*4, %esp                      /* remove xen-provided frame */
+     jmp     sysenter_past_esp
+#endif
+
 ENTRY(entry_SYSENTER_32)
      movl    TSS_sysenter_sp0(%esp), %esp
 sysenter_past_esp:

Can you do this below (ontop of your diff) and get rid of those __{begin,end}_SYSENTER_singlestep_region and __end_entry_SYSENTER_compat globals and use the entry_SYSENTER_{32|compat} symbols instead?

From a quick scan, kallsyms can give you the symbol size too so that you can compute where it ends:

readelf -a vmlinux | grep entry_SYSENTER

 55454: ffffffff8170aff0    99 FUNC    GLOBAL DEFAULT    1 entry_SYSENTER_compat
 62596: ffffffff8170b053     0 NOTYPE  GLOBAL DEFAULT    1 __end_entry_SYSENTER_comp

0xffffffff8170aff0 + 99 = 0xffffffff8170b053

---
Index: b/arch/x86/entry/entry_32.S
===================================================================

arch/x86/entry/entry_32.S 2016-03-06 17:46:52.235733410 +0100 renamed from arch/x86/entry/entry_32.S 2016-03-06 17:47:10.059733163 +0100 +1 -5

@@ -297,18 +297,13 @@ GLOBAL(__begin_SYSENTER_singlestep_regio
  * will ignore all of the single-step traps generated in this range.
  */
 
+ENTRY(entry_SYSENTER_32)
 #ifdef CONFIG_XEN
-/*
- * Xen doesn't set %esp to be precisely what the normal SYSENTER
- * entry point expects, so fix it up before using the normal path.
- */
-ENTRY(xen_sysenter_target)
        addl    $5*4, %esp                      /* remove xen-provided frame */

arch/x86/entry/entry_32.S 2016-03-06 17:46:52.235733410 +0100 GLOBAL(__begin_SYSENTER_singlestep_regio · line 309

Quoted 10 lines

arch/x86/entry/entry_32.S 2016-03-06 17:46:52.235733410 +0100 renamed from arch/x86/entry/entry_32.S 2016-03-06 17:47:10.059733163 +0100 +2 -3

        jmp     sysenter_past_esp
-#endif
-
-ENTRY(entry_SYSENTER_32)
+#else
        movl    TSS_sysenter_sp0(%esp), %esp
+#endif
 sysenter_past_esp:
        pushl   $__USER_DS              /* pt_regs->ss */
        pushl   %ebp                    /* pt_regs->sp (stashed in bp) */

This won't work if you run a Xen enabled kernel on bare metal. This would work though:

ALTERNATIVE "movl TSS_sysenter_sp0(%esp), %esp", "addl $5*4, %esp", X86_FEATURE_XENPV

I haven't read the Xen hypervisor code, but what are those 5 words that were pushed on the stack by the hypervisor? It suspiciously is the size of an IRET frame.

Considering that we don't use SYSEXIT on
Xen anymore, can we just redirect SYSENTER to the INT80 handler?
Perhaps even just disable SYSENTER support in the VDSO on Xen. I
can't imagine SYSENTER is any faster than INT80 on Xen, because it has to trap to the hypervisor first.

Quoted 2 lines

ALTERNATIVE "movl TSS_sysenter_sp0(%esp), %esp", "addl $5*4, %esp", X86_FEATURE_XENPV

That might work.

Quoted 4 lines

I haven't read the Xen hypervisor code, but what are those 5 words that were pushed on the stack by the hypervisor? It suspiciously is the size of an IRET frame.

I think it is nominally an IRET frame.

One might wonder what's in it, given that the hypervisor doesn't know what the old IP or SP was.

Quoted 7 lines

Considering that we don't use SYSEXIT on
Xen anymore, can we just redirect SYSENTER to the INT80 handler?
Perhaps even just disable SYSENTER support in the VDSO on Xen. I
can't imagine SYSENTER is any faster than INT80 on Xen, because it has to trap to the hypervisor first.

I think we should leave it enabled -- having user programs on Xen PV trap into the hypervisor for a system call using SYSENTER will still be much faster than using INT $0x80 as long as the hypervisor does something reasonable.

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

Quoted 11 lines · Andy Lutomirski

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

What is wrong with the current method of clearing TF and setting TIF_SINGLESTEP on the first debug trap? This patch actually increases complexity because it has to check for a range of addresses rather than just the first instruction, plus it has to singlestep all the way through the SYSENTER prologue.

Unless there is an actual issue with TIF_SINGLESTEP, I don't think this patch is an improvement.

Quoted 13 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

Quoted 8 lines

What is wrong with the current method of clearing TF and setting TIF_SINGLESTEP on the first debug trap? This patch actually increases complexity because it has to check for a range of addresses rather than just the first instruction, plus it has to singlestep all the way through the SYSENTER prologue.

Unless there is an actual issue with TIF_SINGLESTEP, I don't think this patch is an improvement.

TIF_SINGLESTEP has bizarrely overloaded semantics.

There's this:

/*
* If we stepped into a sysenter/syscall insn, it trapped in
* kernel mode; do_debug() cleared TF and set TIF_SINGLESTEP.
* If user-mode had set TF itself, then it's still clear from
* do_debug() and we need to set it again to restore the user
* state so we don't wrongly set TIF_FORCED_TF below.
* If enable_single_step() was used last and that is what
* set TIF_SINGLESTEP, then both TF and TIF_FORCED_TF are
* already set and our bookkeeping is fine.
*/
if (unlikely(test_tsk_thread_flag(child, TIF_SINGLESTEP)))
regs->flags |= X86_EFLAGS_TF;

but TIF_SINGLESTEP is also used for other things. (And I need to follow up with a patch to remove that comment.) This results in incomprehensible behavior: if a user program sets TF and does SYSENTER, then TIF_SINGLESTEP gets set (and stays set!). This does not happen if a user sets TF and does INT80 or SYSCALL. There was at least one bug in here that Oleg fixed a while back, and I wouldn't be at all surprised if there were others.

I don't know what would happen if TF were set and SYSENTER were used to do a syscall where the __get_user to load the syscall nr failed. That happens before the TIF_SINGLESTEP fixup.

Basically, the overloaded use of TIF_SINGLESTEP was complicated and hard to understand, and the new behavior is straightforward and consistent with other entries, even if it's a bit slower.

We could introduce a new TIF_SYSENTER_TF and use it directly, or we could accelerate the TF fixup in regs->flags by switching to a different entry path (I had a draft patch to do that), but I tend to favor simplicity for things that aren't performance-critical.

Quoted 56 lines · Andy Lutomirski

Quoted 13 lines · Brian Gerst

Quoted 11 lines · Andy Lutomirski

Due to a blatant design error, SYSENTER doesn't clear TF. As a result, if a user does SYSENTER with TF set, we will single-step through the kernel until something clears TF. There is absolutely nothing we can do to prevent this short of turning off SYSENTER [1].

Simplify the handling considerably with two changes:

1. We already sanitize EFLAGS in SYSENTER to clear NT and AC. We can add TF to that list of flags to sanitize with no overhead whatsoever.

2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.

Quoted 8 lines

What is wrong with the current method of clearing TF and setting TIF_SINGLESTEP on the first debug trap? This patch actually increases complexity because it has to check for a range of addresses rather than just the first instruction, plus it has to singlestep all the way through the SYSENTER prologue.

Unless there is an actual issue with TIF_SINGLESTEP, I don't think this patch is an improvement.

TIF_SINGLESTEP has bizarrely overloaded semantics.

There's this:

/*
* If we stepped into a sysenter/syscall insn, it trapped in
* kernel mode; do_debug() cleared TF and set TIF_SINGLESTEP.
* If user-mode had set TF itself, then it's still clear from
* do_debug() and we need to set it again to restore the user
* state so we don't wrongly set TIF_FORCED_TF below.
* If enable_single_step() was used last and that is what
* set TIF_SINGLESTEP, then both TF and TIF_FORCED_TF are
* already set and our bookkeeping is fine.
*/
if (unlikely(test_tsk_thread_flag(child, TIF_SINGLESTEP)))
regs->flags |= X86_EFLAGS_TF;

but TIF_SINGLESTEP is also used for other things. (And I need to follow up with a patch to remove that comment.) This results in incomprehensible behavior: if a user program sets TF and does SYSENTER, then TIF_SINGLESTEP gets set (and stays set!). This does not happen if a user sets TF and does INT80 or SYSCALL. There was at least one bug in here that Oleg fixed a while back, and I wouldn't be at all surprised if there were others.

I don't know what would happen if TF were set and SYSENTER were used to do a syscall where the __get_user to load the syscall nr failed. That happens before the TIF_SINGLESTEP fixup.

Basically, the overloaded use of TIF_SINGLESTEP was complicated and hard to understand, and the new behavior is straightforward and consistent with other entries, even if it's a bit slower.

Quoted 4 lines

We could introduce a new TIF_SYSENTER_TF and use it directly, or we could accelerate the TF fixup in regs->flags by switching to a different entry path (I had a draft patch to do that), but I tend to favor simplicity for things that aren't performance-critical.

The alternate entry path wouldn't be very big, just 5 instructions with the OR being the only difference.

Another option is to use a per-cpu var instead of a TIF flag.

Right after SYSENTER, we can get a #DB or NMI. On x86_32, there's no IST, so the exception handler is invoked on the temporary SYSENTER stack.

Because the SYSENTER stack is very small, we have a fixup to switch off the stack quickly when this happens. The old fixup had several issues:

1. It checked the interrupt frame's CS and EIP. This wasn't obviously correct on Xen or if vm86 mode was in use [1].

2. In the NMI handler, it did some frightening digging into the stack frame. I'm not convinced this digging was correct.

3. The fixup didn't switch stacks and then switch back. Instead, it synthesized a brand new stack frame that would redirect the IRET back to the SYSENTER code. That frame was highly questionable. For one thing, if NMI nested inside #DB, we would effectively abort the #DB prologue, which was probably safe but was frightening. For another, the code used PUSHFL to write the FLAGS portion of the frame, which was simply bogus -- by the time PUSHFL was called, at least TF, NT, VM, and all of the arithmetic flags were clobbered.

Simplify this considerably. Instead of looking at the saved frame to see where we came from, check the hardware ESP register against the SYSENTER stack directly. Malicious user code cannot spoof the kernel ESP register, and by moving the check after SAVE_ALL, we can use normal PER_CPU accesses to find all the relevant addresses.

With this patch applied, the improved syscall_nt_32 test finally passes on 32-bit kernels.

[1] It isn't obviously correct, but it is nonetheless safe from vm86 shenanigans as far as I can tell. A user can't point EIP at entry_SYSENTER_32 while in vm86 mode because entry_SYSENTER_32, like all kernel addresses, is greater than 0xffff and would thus violate the CS segment limit.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_32.S        | 114 ++++++++++++++++++---------------------
 arch/x86/kernel/asm-offsets_32.c |   5 ++
 2 files changed, 56 insertions(+), 63 deletions(-)

arch/x86/entry/entry_32.S +0 -23

index 7700cf695d8c..ad9a85e62128 100644
@@ -987,51 +987,48 @@ error_code:
 	jmp	ret_from_exception
 END(page_fault)
 
-/*
- * Debug traps and NMI can happen at the one SYSENTER instruction
- * that sets up the real kernel stack. Check here, since we can't
- * allow the wrong stack to be used.
- *
- * "TSS_sysenter_sp0+12" is because the NMI/debug handler will have
- * already pushed 3 words if it hits on the sysenter instruction:
- * eflags, cs and eip.
- *
- * We just load the right stack, and push the three (known) values
- * by hand onto the new stack - while updating the return eip past
- * the instruction that would have done it for sysenter.
- */
-.macro FIX_STACK offset ok label
-	cmpw	$__KERNEL_CS, 4(%esp)
-	jne	\ok
-\label:
-	movl	TSS_sysenter_sp0 + \offset(%esp), %esp
-	pushfl
-	pushl	$__KERNEL_CS
-	pushl	$sysenter_past_esp
-.endm
-

arch/x86/entry/entry_32.S +0 -3

 	ASM_CLAC
-	cmpl	$entry_SYSENTER_32, (%esp)
-	jne	debug_stack_correct
-	FIX_STACK 12, debug_stack_correct, debug_esp_fix_insn

arch/x86/entry/entry_32.S +18 -21

+	cmpl	$SIZEOF_SYSENTER_stack, %ecx
+	jb	.Ldebug_from_sysenter_stack
+
+	TRACE_IRQS_OFF
+	call	do_debug
+	jmp	ret_from_exception
+
+.Ldebug_from_sysenter_stack:
+	/* We're on the SYSENTER stack.  Switch off. */
+	movl	%esp, %ebp
+	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esp
+	TRACE_IRQS_OFF
 	call	do_debug
+	movl	%ebp, %esp
 	jmp	ret_from_exception
 END(debug)
 
 /*
- * NMI is doubly nasty. It can happen _while_ we're handling
- * a debug fault, and the debug fault hasn't yet been able to
- * clear up the stack. So we first check whether we got  an
- * NMI on the sysenter entry path, but after that we need to
- * check whether we got an NMI on the debug path where the debug
- * fault happened on the sysenter path.
+ * NMI is doubly nasty.  It can happen on the first instruction of
+ * entry_SYSENTER_32 (just like #DB), but it can also interrupt the beginning
+ * of the #DB handler even if that #DB in turn hit before entry_SYSENTER_32
+ * switched stacks.  We handle both conditions by simply checking whether we
+ * interrupted kernel code running on the SYSENTER stack.
  */
 ENTRY(nmi)
 	ASM_CLAC

@@ -1042,41 +1039,32 @@ ENTRY(nmi)
 	popl	%eax
 	je	nmi_espfix_stack
 #endif
-	cmpl	$entry_SYSENTER_32, (%esp)
-	je	nmi_stack_fixup
-	pushl	%eax
-	movl	%esp, %eax
-	/*
-	 * Do not access memory above the end of our stack page,
-	 * it might not exist.
-	 */
-	andl	$(THREAD_SIZE-1), %eax
-	cmpl	$(THREAD_SIZE-20), %eax
-	popl	%eax
-	jae	nmi_stack_correct
-	cmpl	$entry_SYSENTER_32, 12(%esp)
-	je	nmi_debug_stack_check
-nmi_stack_correct:

arch/x86/entry/entry_32.S +14 -13

+	cmpl	$SIZEOF_SYSENTER_stack, %ecx
+	jb	.Lnmi_from_sysenter_stack
+
+	/* Not on SYSENTER stack. */
 	call	do_nmi
 	jmp	restore_all_notrace
 
-nmi_stack_fixup:
-	FIX_STACK 12, nmi_stack_correct, 1
-	jmp	nmi_stack_correct
-
-nmi_debug_stack_check:
-	cmpw	$__KERNEL_CS, 16(%esp)
-	jne	nmi_stack_correct
-	cmpl	$debug, (%esp)
-	jb	nmi_stack_correct
-	cmpl	$debug_esp_fix_insn, (%esp)
-	ja	nmi_stack_correct
-	FIX_STACK 24, nmi_stack_correct, 1
-	jmp	nmi_stack_correct
+.Lnmi_from_sysenter_stack:
+	/*
+	 * We're on the SYSENTER stack.  Switch off.  No one (not even debug)
+	 * is using the thread stack right now, so it's safe for us to use it.
+	 */
+	movl	%esp, %ebp
+	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esp
+	call	do_nmi
+	movl	%ebp, %esp
+	jmp	restore_all_notrace
 
 #ifdef CONFIG_X86_ESPFIX32
 nmi_espfix_stack:

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 3 ++-
 arch/x86/kernel/process.c        | 3 +++
 arch/x86/kernel/traps.c          | 8 ++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

arch/x86/include/asm/processor.h +2 -1

index 7cd01b71b5bd..50a6dc871cc0 100644
@@ -299,8 +299,9 @@ struct tss_struct {
 
 #ifdef CONFIG_X86_32
 	/*
-	 * Space for the temporary SYSENTER stack:
+	 * Space for the temporary SYSENTER stack.
 	 */
+	unsigned long		SYSENTER_stack_canary;
 	unsigned long		SYSENTER_stack[64];
 #endif
 

arch/x86/kernel/process.c +3 -0

index 9f7c21c22477..ee9a9792caeb 100644
@@ -57,6 +57,9 @@ __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
 	  */
 	.io_bitmap		= { [0 ... IO_BITMAP_LONGS] = ~0 },
 #endif
+#ifdef CONFIG_X86_32
+	.SYSENTER_stack_canary	= STACK_END_MAGIC,
+#endif
 };
 EXPORT_PER_CPU_SYMBOL(cpu_tss);
 

arch/x86/kernel/traps.c +8 -0

index 80928ea78373..590110119e6a 100644
@@ -713,6 +713,14 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	debug_stack_usage_dec();
 
 exit:
+#if defined(CONFIG_X86_32)
+	/*
+	 * This is the most likely code path that involves non-trivial use
+	 * of the SYSENTER stack.  Check that we haven't overrun it.
+	 */
+	WARN(this_cpu_read(cpu_tss.SYSENTER_stack_canary) != STACK_END_MAGIC,
+	     "Overran or corrupted SYSENTER stack\n");
+#endif
 	ist_exit(regs);
 }
 NOKPROBE_SYMBOL(do_debug);

Signature · 1 line

2.5.0

On Sat, Mar 05, 2016 at 09:52:23PM -0800, Andy Lutomirski wrote:

<--- ?

So all patches before are nice and verbose in explaining what they do. Where did the commit message of this one disappear?

:-)

Or is it that obvious that we should have a stack canary? How about some text *why* we need it?

This series applies to the result of merging tip:x86/asm and tip:x86/urgent. I've been testing on a somewhat bastardized base, because tip currently doesn't work on my laptop in 32-bit mode. (That bug is fixed in Linus' tree.)

Quoted 4 lines · Andy Lutomirski

This series applies to the result of merging tip:x86/asm and tip:x86/urgent. I've been testing on a somewhat bastardized base, because tip currently doesn't work on my laptop in 32-bit mode. (That bug is fixed in Linus' tree.)

Hm, which fix is that?

Quoted 4 lines · Ingo Molnar

* Andy Lutomirski <[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

This series applies to the result of merging tip:x86/asm and tip:x86/urgent. I've been testing on a somewhat bastardized base, because tip currently doesn't work on my laptop in 32-bit mode. (That bug is fixed in Linus' tree.)

Hm, which fix is that?

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=de9e478b9d49f3a0214310d921450cf5bb4a21e6

hpa asked me to get rid of the ASM_CLAC at the beginning of the SYSENTER path. Little did he know...

Quoted 2 lines · Andy Lutomirski

hpa asked me to get rid of the ASM_CLAC at the beginning of the SYSENTER path. Little did he know...

Btw., before we further change this code, something else I think would be very useful. We have countless system call entry points on x86 CPUs, and they are now consistently named and are very easy to grep for:

triton:~/tip> git grep 'ENTRY(entry_' arch/x86/entry/ arch/x86/entry/entry_32.S:ENTRY(entry_SYSENTER_32) arch/x86/entry/entry_32.S:ENTRY(entry_INT80_32) arch/x86/entry/entry_64.S:ENTRY(entry_SYSCALL_64) arch/x86/entry/entry_64_compat.S:ENTRY(entry_SYSENTER_compat) arch/x86/entry/entry_64_compat.S:ENTRY(entry_SYSCALL_compat) arch/x86/entry/entry_64_compat.S:ENTRY(entry_INT80_compat)

Furthermore, each entry point has extensive comments, except one important detail:
none of the comments really explains the circumstances under which the entry points are _used_ by user-space.

I'd like to see something like:

arch/x86/entry/entry_64.S:ENTRY(entry_SYSCALL_64)

*
* The 64-bit SYSCALL instruction is used by all modern 64-bit user-space
* code to execute most system calls: this instruction is the fastest and
* sanest implementation on modern Intel and AMD CPUs.
*

... and we should add similar explanations for all of the 6 entry points, with caveats and limitations listed generously.

Especially valuable would be to list eventual 'strange' usages of the various syscall instructions, used by rare packages, compatibility layers, emulators, embedded libraries, etc. (To the extent we know about them, obviously.)