Skip to contentSkip to conversation

x86_64: Make int3 non-magical

PATCH In review Andy Lutomirski 4 participants 13 messages Jul 25, 2015 – Aug 19, 2015

Subsystems (2)
lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

v2 cover x86_64: Make int3 non-magical

int3 uses IST and the paranoid gsbase path. Neither is necessary, although the IST stack may currently be necessary to avoid stack overruns.

Clean up IRQ stacks, make them NMI safe, teach idtentry to use irqstacks if requested, and move int3 to the IRQ stack.

This prepares us to return from int3 using RET. While we could, in principle, return from an IST entry using RET, making that work seems likely to be much messier and more fragile than this approach.

Changes from v1: Actually NMI-safe

Andy Lutomirski (3):
x86/entry/64: Refactor IRQ stacks and make them NMI-safe x86/entry/64: Teach idtentry to use the IRQ stack x86/entry/64: Move #BP from IST to the IRQ stack

 arch/x86/Kconfig.debug       |   2 -
 arch/x86/entry/entry_64.S    | 105 ++++++++++++++++++++++++++++++-------------
 arch/x86/kernel/process_64.c |   4 ++
 arch/x86/kernel/traps.c      |  26 +++++------
 4 files changed, 90 insertions(+), 47 deletions(-)
Signature · 1 line
2.4.3
author maintainer
#1 lore raw

PATCH v2 1/3 x86/entry/64: Refactor IRQ stacks and make them NMI-safe

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

---
 arch/x86/Kconfig.debug       |  2 -
 arch/x86/entry/entry_64.S    | 89 +++++++++++++++++++++++++++++---------------
 arch/x86/kernel/process_64.c |  4 ++
 3 files changed, 63 insertions(+), 32 deletions(-)
arch/x86/Kconfig.debug +0 -2
index d8c0d3266173..4196e2325971 100644
@@ -305,8 +305,6 @@ config DEBUG_ENTRY
 	  Some of these sanity checks may slow down kernel entries and
 	  exits or otherwise impact performance.
 
-	  This is currently used to help test NMI code.
-
 	  If unsure, say N.
 
 config DEBUG_NMI_SELFTEST
arch/x86/entry/entry_64.S +59 -30

131 lines not on this page · raw message

arch/x86/kernel/process_64.c +4 -0
index 0831ba3bcf95..70aa5917456e 100644
@@ -280,6 +280,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	unsigned fsindex, gsindex;
 	fpu_switch_t fpu_switch;
 
+#ifdef CONFIG_DEBUG_ENTRY
+	WARN_ON_ONCE(this_cpu_read(irq_count) != -1);
+#endif
+
 	fpu_switch = switch_fpu_prepare(prev_fpu, next_fpu, cpu);
 
 	/* We must save %fs and %gs before load_TLS() because
Signature · 1 line
2.4.3
maintainer
#2 lore raw

Re: [PATCH v2 1/3] x86/entry/64: Refactor IRQ stacks and make them NMI-safe

Quoted 8 lines · Andy Lutomirski

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

Quoted 6 lines
---
 arch/x86/Kconfig.debug       |  2 -
 arch/x86/entry/entry_64.S    | 89 +++++++++++++++++++++++++++++---------------
 arch/x86/kernel/process_64.c |  4 ++
 3 files changed, 63 insertions(+), 32 deletions(-)

What does that apply against?

I get conflicts against current Linus master:

$ patch -p1 --dry-run -i /tmp/0001-x86-entry-64-refactor_irq_stacks_and_make_them_nmi-safe.patch 
checking file arch/x86/Kconfig.debug
checking file arch/x86/entry/entry_64.S
Hunk #1 succeeded at 544 (offset 54 lines).
Hunk #2 FAILED at 568.
Hunk #3 FAILED at 598.
Hunk #4 succeeded at 1012 (offset 99 lines).
Hunk #5 succeeded at 1033 (offset 99 lines).
2 out of 5 hunks FAILED
checking file arch/x86/kernel/process_64.c
Signature · 5 lines
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 1/3] x86/entry/64: Refactor IRQ stacks and make them NMI-safe

Quoted 16 lines · Borislav Petkov
Quoted 8 lines · Andy Lutomirski

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

Quoted 6 lines
---
 arch/x86/Kconfig.debug       |  2 -
 arch/x86/entry/entry_64.S    | 89 +++++++++++++++++++++++++++++---------------
 arch/x86/kernel/process_64.c |  4 ++
 3 files changed, 63 insertions(+), 32 deletions(-)
Quoted 3 lines

What does that apply against?

I get conflicts against current Linus master:

It applies to the result of merging Linus' master with tip/x86/asm. I think it also applies to tip/x86/asm directly, but I haven't tried it. It's an NMI-related thing (sort of), so it seemed sensible to develop against something that has the other NMI fixes.

--Andy

maintainer Reviewed-by on v2 1/3
#4 lore raw

Re: [PATCH v2 1/3] x86/entry/64: Refactor IRQ stacks and make them NMI-safe

In reply to Andy Lutomirski
Quoted 8 lines · Andy Lutomirski

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

Quoted 6 lines
---
 arch/x86/Kconfig.debug       |  2 -
 arch/x86/entry/entry_64.S    | 89 +++++++++++++++++++++++++++++---------------
 arch/x86/kernel/process_64.c |  4 ++
 3 files changed, 63 insertions(+), 32 deletions(-)
Signature · 5 lines
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 1/3] x86/entry/64: Refactor IRQ stacks and make them NMI-safe

In reply to Andy Lutomirski
Quoted 9 lines · Andy Lutomirski

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

Quoted 7 lines

+/*
+ * Enters the IRQ stack if we're not already using it. NMI-safe. Clobbers
+ * flags and puts old RSP into old_rsp, and leaves all other GPRs alone.
+ * Requires kernel GSBASE.
+ *
+ * The invariant is that, if irq_count != 1, then the IRQ stack is in use.
+ */

I might be wrong, but shouldn't be this read as 'if irq_count != -1' ?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#6 lore raw

Re: [PATCH v2 1/3] x86/entry/64: Refactor IRQ stacks and make them NMI-safe

Quoted 18 lines · Andy Shevchenko
Quoted 9 lines · Andy Lutomirski

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

Quoted 7 lines

+/*
+ * Enters the IRQ stack if we're not already using it. NMI-safe. Clobbers
+ * flags and puts old RSP into old_rsp, and leaves all other GPRs alone.
+ * Requires kernel GSBASE.
+ *
+ * The invariant is that, if irq_count != 1, then the IRQ stack is in use.
+ */

Quoted 2 lines

I might be wrong, but shouldn't be this read as 'if irq_count != -1' ?

Indeed. Will fix for the next version.

--Andy

author maintainer
#7 lore raw

PATCH v2 2/3 x86/entry/64: Teach idtentry to use the IRQ stack

In reply to Andy Lutomirski

We don't specifically need IST for things like kprobes, but we do want to avoid rare, surprising extra stack usage if a kprobe hits with a deep stack.

Teach idtentry to use the IRQ stack for selected entries.

This implementation uses the IRQ stack even if we entered from user mode. This disallows tricks like ist_begin_non_atomic. If we ever need such a trick in one of these entries, we can rework this. For now, let's keep it simple.

---
 arch/x86/entry/entry_64.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +13 -1
index 3227fb6a5b94..af3573e75ed4 100644
@@ -756,13 +756,17 @@ apicinterrupt IRQ_WORK_VECTOR			irq_work_interrupt		smp_irq_work_interrupt
  */
 #define CPU_TSS_IST(x) PER_CPU_VAR(cpu_tss) + (TSS_ist + ((x) - 1) * 8)
 
-.macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1
+.macro idtentry sym do_sym has_error_code:req irqstack=0 paranoid=0 shift_ist=-1
 ENTRY(\sym)
 	/* Sanity check */
 	.if \shift_ist != -1 && \paranoid == 0
 	.error "using shift_ist requires paranoid=1"
 	.endif
 
+	.if \irqstack && \paranoid
+	.error "using irqstack requires !paranoid"
+	.endif
+
 	ASM_CLAC
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 
@@ -804,8 +808,16 @@ ENTRY(\sym)
 	subq	$EXCEPTION_STKSZ, CPU_TSS_IST(\shift_ist)
 	.endif
 
+	.if \irqstack
+	ENTER_IRQ_STACK old_rsp=%rcx
+	.endif
+
 	call	\do_sym
 
+	.if \irqstack
+	LEAVE_IRQ_STACK
+	.endif
+
 	.if \shift_ist != -1
 	addq	$EXCEPTION_STKSZ, CPU_TSS_IST(\shift_ist)
 	.endif
Signature · 1 line
2.4.3
maintainer Reviewed-by on v2 2/3
#8 lore raw

Re: [PATCH v2 2/3] x86/entry/64: Teach idtentry to use the IRQ stack

Quoted 11 lines · Andy Lutomirski

We don't specifically need IST for things like kprobes, but we do want to avoid rare, surprising extra stack usage if a kprobe hits with a deep stack.

Teach idtentry to use the IRQ stack for selected entries.

This implementation uses the IRQ stack even if we entered from user mode. This disallows tricks like ist_begin_non_atomic. If we ever need such a trick in one of these entries, we can rework this. For now, let's keep it simple.

Quoted 4 lines
---
 arch/x86/entry/entry_64.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
author maintainer
#9 lore raw

PATCH v2 3/3 x86/entry/64: Move #BP from IST to the IRQ stack

In reply to Andy Lutomirski

There's nothing IST-worthy about #BP/int3. We don't allow kprobes in the small handful of places in the kernel that run at CPL0 with an invalid stack, and 32-bit kernels have used normal interrupt gates for #BP forever.

Furthermore, we don't allow kprobes in places that have usergs while in kernel mode, so "paranoid" is also unnecessary.

---
 arch/x86/entry/entry_64.S |  2 +-
 arch/x86/kernel/traps.c   | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)
arch/x86/entry/entry_64.S +1 -1
index af3573e75ed4..e353709d1fcc 100644
@@ -1007,7 +1007,7 @@ apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
 #endif /* CONFIG_HYPERV */
 
 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 int3			do_int3			has_error_code=0	irqstack=1
 idtentry stack_segment		do_stack_segment	has_error_code=1
 
 #ifdef CONFIG_XEN
arch/x86/kernel/traps.c +13 -13
index 8e65d8a9b8db..d823db70f492 100644
@@ -479,7 +479,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 }
 NOKPROBE_SYMBOL(do_general_protection);
 
-/* May run on IST stack. */
+/* Runs on IRQ stack. */
 dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 {
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -494,7 +494,15 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 	if (poke_int3_handler(regs))
 		return;
 
+	/*
+	 * Use ist_enter despite the fact that we don't use an IST stack.
+	 * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
+	 * mode or even during context tracking state changes.
+	 *
+	 * This means that we can't schedule.  That's okay.
+	 */
 	ist_enter(regs);
+
 	CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
 	if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,
@@ -511,15 +519,10 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 			SIGTRAP) == NOTIFY_STOP)
 		goto exit;
 
-	/*
-	 * Let others (NMI) know that the debug stack is in use
-	 * as we may switch to the interrupt stack.
-	 */
-	debug_stack_usage_inc();
 	preempt_conditional_sti(regs);
 	do_trap(X86_TRAP_BP, SIGTRAP, "int3", regs, error_code, NULL);
 	preempt_conditional_cli(regs);
-	debug_stack_usage_dec();
+
 exit:
 	ist_exit(regs);
 }
@@ -885,19 +888,16 @@ void __init trap_init(void)
 	cpu_init();
 
 	/*
-	 * X86_TRAP_DB and X86_TRAP_BP have been set
-	 * in early_trap_init(). However, ITS works only after
-	 * cpu_init() loads TSS. See comments in early_trap_init().
+	 * X86_TRAP_DB was installed in early_trap_init(). However,
+	 * IST works only after cpu_init() loads TSS. See comments
+	 * in early_trap_init().
 	 */
 	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
-	/* int3 can be called from all */
-	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
 
 	x86_init.irqs.trap_init();
 
 #ifdef CONFIG_X86_64
 	memcpy(&debug_idt_table, &idt_table, IDT_ENTRIES * 16);
 	set_nmi_gate(X86_TRAP_DB, &debug);
-	set_nmi_gate(X86_TRAP_BP, &int3);
 #endif
 }
Signature · 1 line
2.4.3
maintainer
#10 lore raw

Re: [PATCH v2 3/3] x86/entry/64: Move #BP from IST to the IRQ stack

Quoted 13 lines · Andy Lutomirski

There's nothing IST-worthy about #BP/int3. We don't allow kprobes in the small handful of places in the kernel that run at CPL0 with an invalid stack, and 32-bit kernels have used normal interrupt gates for #BP forever.

Furthermore, we don't allow kprobes in places that have usergs while in kernel mode, so "paranoid" is also unnecessary.

---
 arch/x86/entry/entry_64.S |  2 +-
 arch/x86/kernel/traps.c   | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

...

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

Quoted 12 lines
arch/x86/kernel/traps.c +7 -0
@@ -494,7 +494,15 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 	if (poke_int3_handler(regs))
 		return;
 
+	/*
+	 * Use ist_enter despite the fact that we don't use an IST stack.
+	 * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
+	 * mode or even during context tracking state changes.
+	 *
+	 * This means that we can't schedule.  That's okay.
+	 */
 	ist_enter(regs);

Let's rename that thing. Call it atomic_ctxt_enter or whatever...

Signature · 5 lines
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 3/3] x86/entry/64: Move #BP from IST to the IRQ stack

Quoted 21 lines · Borislav Petkov
Quoted 13 lines · Andy Lutomirski

There's nothing IST-worthy about #BP/int3. We don't allow kprobes in the small handful of places in the kernel that run at CPL0 with an invalid stack, and 32-bit kernels have used normal interrupt gates for #BP forever.

Furthermore, we don't allow kprobes in places that have usergs while in kernel mode, so "paranoid" is also unnecessary.

---
 arch/x86/entry/entry_64.S |  2 +-
 arch/x86/kernel/traps.c   | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

...

Quoted 4 lines
arch/x86/kernel/traps.c +0 -0
@@ -494,7 +494,15 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
      if (poke_int3_handler(regs))
              return;
 
Quoted 11 lines

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

Quoted 8 lines
arch/x86/kernel/traps.c +7 -0
+     /*
+      * Use ist_enter despite the fact that we don't use an IST stack.
+      * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
+      * mode or even during context tracking state changes.
+      *
+      * This means that we can't schedule.  That's okay.
+      */
      ist_enter(regs);

Let's rename that thing. Call it atomic_ctxt_enter or whatever...

OK if I do that as a follow-up? It would probably want to be a separate patch anyway.

Hmm, I'm starting to like this new regime in which we never ever switch to user mode from anywhere other than the standard kernel stack. It looks like even Xen may play along and do it cleanly soon :) Maybe I'll even add an assertion somewhere to make sure we don't break it. (I think this also means that the bad iret fixup can be simplified.)

Also, with all this stuff applied (and the modify_ldt thing, once the Xen folks figure out what's wrong), I think we can reinstate the old LARL check for 16-bit segments and thus prevent naughty users from banging on espfix using only sigreturn.

--Andy

maintainer
#12 lore raw

Re: [PATCH v2 3/3] x86/entry/64: Move #BP from IST to the IRQ stack

Quoted 2 lines · Andy Lutomirski

OK if I do that as a follow-up? It would probably want to be a separate patch anyway.

Of course.

Quoted 6 lines

Hmm, I'm starting to like this new regime in which we never ever switch to user mode from anywhere other than the standard kernel stack. It looks like even Xen may play along and do it cleanly soon :) Maybe I'll even add an assertion somewhere to make sure we don't break it. (I think this also means that the bad iret fixup can be simplified.)

Definitely sounds like a nice, logical thing. We sometimes switch stacks to land on the kernel stack before returning to user mode (IST and all) but I guess that's a clean enough thing to do. Oh, and only a couple of insns so yeah.

Quoted 4 lines

Also, with all this stuff applied (and the modify_ldt thing, once the Xen folks figure out what's wrong), I think we can reinstate the old LARL check for 16-bit segments and thus prevent naughty users from banging on espfix using only sigreturn.

Uuh, and then only check ZF. I guess this should cover all the legacy cases, which is nice.

Yeah, sounds coolio. :-)

Signature · 5 lines
Regards/Gruss,
    Boris.

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