Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423599 > unrolled thread

[PATCH 00/13] Virtually mapped stacks with guard pages (x86, core)

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:30 +0200
    [PATCH 01/13] x86/mm/hotplug: Don't remove PGD entries in remove_pagetable() Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:30 +0200
    [PATCH 12/13] x86/mm/64: Enable vmapped stacks Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:30 +0200
      Re: [PATCH 12/13] x86/mm/64: Enable vmapped stacks Andy Lutomirski <luto@amacapital.net> - 2016-06-16 07:40 +0200
        Re: [kernel-hardening] Re: [PATCH 12/13] x86/mm/64: Enable vmapped  stacks Rik van Riel <riel@redhat.com> - 2016-06-16 15:20 +0200
    [PATCH 03/13] x86/cpa: Warn if kernel_unmap_pages_in_pgd is used inappropriately Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:30 +0200
    [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack before do_exit Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:40 +0200
      Re: [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack  before do_exit Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-16 20:00 +0200
        Re: [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack  before do_exit Andy Lutomirski <luto@amacapital.net> - 2016-06-16 20:00 +0200
    [PATCH 02/13] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:40 +0200
    [PATCH 06/13] fork: Add generic vmalloced stack support Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:40 +0200
      Re: [PATCH 06/13] fork: Add generic vmalloced stack support Kees Cook <keescook@chromium.org> - 2016-06-16 19:30 +0200
        Re: [PATCH 06/13] fork: Add generic vmalloced stack support Andy Lutomirski <luto@amacapital.net> - 2016-06-16 19:40 +0200
    [PATCH 07/13] x86/die: Don't try to recover from an OOPS on a non-default stack Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:40 +0200
    [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow Andy Lutomirski <luto@kernel.org> - 2016-06-16 02:40 +0200
      Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on  stack overflow Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-16 20:20 +0200
        Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on  stack overflow Andy Lutomirski <luto@amacapital.net> - 2016-06-16 20:30 +0200
          Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on  stack overflow Andy Lutomirski <luto@amacapital.net> - 2016-06-16 20:40 +0200
            Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on  stack overflow Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-16 21:00 +0200
          Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on  stack overflow Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-16 20:40 +0200
    Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86,  core) Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-06-16 08:10 +0200
      Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-16 20:00 +0200
        Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-16 20:20 +0200
          Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-16 23:30 +0200
      Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-17 06:00 +0200
        Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86,  core) Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-06-17 09:30 +0200
          Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-17 19:40 +0200
            Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86,  core) Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-06-20 08:00 +0200
              Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Andy Lutomirski <luto@amacapital.net> - 2016-06-20 08:10 +0200
                Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86,  core) Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-06-20 09:40 +0200
    Re: [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core) Kees Cook <keescook@chromium.org> - 2016-06-16 19:30 +0200

Page 1 of 2  [1] 2  Next page →

#1423599 — [PATCH 00/13] Virtually mapped stacks with guard pages (x86, core)

Since the dawn of time, a kernel stack overflow has been a real PITA
to debug, has caused nondeterministic crashes some time after the
actual overflow, and has generally been easy to exploit for root.

With this series, arches can enable HAVE_ARCH_VMAP_STACK.  Arches
that enable it (just x86 for now) get virtually mapped stacks with
guard pages.  This causes reliable faults when the stack overflows.

If the arch implements it well, we get a nice OOPS on stack overflow
(as opposed to panicing directly or otherwise exploding badly).  On
x86, the OOPS is nice, has a usable call trace, and the overflowing
task is killed cleanly.

This does not address interrupt stacks.

Andy Lutomirski (12):
  x86/cpa: In populate_pgd, don't set the pgd entry until it's populated
  x86/cpa: Warn if kernel_unmap_pages_in_pgd is used inappropriately
  mm: Track NR_KERNEL_STACK in pages instead of number of stacks
  mm: Move memcg stack accounting to account_kernel_stack
  fork: Add generic vmalloced stack support
  x86/die: Don't try to recover from an OOPS on a non-default stack
  x86/dumpstack: When OOPSing, rewind the stack before do_exit
  x86/dumpstack: When dumping stack bytes due to OOPS, start with
    regs->sp
  x86/dumpstack: Try harder to get a call trace on stack overflow
  x86/dumpstack/64: Handle faults when printing the "Stack:" part of an
    OOPS
  x86/mm/64: Enable vmapped stacks
  x86/mm: Improve stack-overflow #PF handling

Ingo Molnar (1):
  x86/mm/hotplug: Don't remove PGD entries in remove_pagetable()

 arch/Kconfig                     | 12 ++++++++++
 arch/x86/Kconfig                 |  1 +
 arch/x86/entry/entry_32.S        | 11 +++++++++
 arch/x86/entry/entry_64.S        | 11 +++++++++
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++-
 arch/x86/include/asm/traps.h     |  6 +++++
 arch/x86/kernel/dumpstack.c      | 17 +++++++++++++-
 arch/x86/kernel/dumpstack_32.c   |  4 +++-
 arch/x86/kernel/dumpstack_64.c   | 16 ++++++++++---
 arch/x86/kernel/traps.c          | 32 +++++++++++++++++++++++++
 arch/x86/mm/fault.c              | 39 ++++++++++++++++++++++++++++++
 arch/x86/mm/init_64.c            | 27 ---------------------
 arch/x86/mm/pageattr.c           |  7 +++++-
 arch/x86/mm/tlb.c                | 15 ++++++++++++
 fs/proc/meminfo.c                |  2 +-
 kernel/fork.c                    | 51 ++++++++++++++++++++++++++++++----------
 mm/page_alloc.c                  |  3 +--
 17 files changed, 233 insertions(+), 49 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1423600 — [PATCH 01/13] x86/mm/hotplug: Don't remove PGD entries in remove_pagetable()

From: Ingo Molnar <mingo@kernel.org>

So when memory hotplug removes a piece of physical memory from pagetable
mappings, it also frees the underlying PGD entry.

This complicates PGD management, so don't do this. We can keep the
PGD mapped and the PUD table all clear - it's only a single 4K page
per 512 GB of memory hotplugged.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Waiman Long <Waiman.Long@hp.com>
Cc: linux-mm@kvack.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Message-Id: <1442903021-3893-4-git-send-email-mingo@kernel.org>
---
 arch/x86/mm/init_64.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index bce2e5d9edd4..c7465453d64e 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -702,27 +702,6 @@ static void __meminit free_pmd_table(pmd_t *pmd_start, pud_t *pud)
 	spin_unlock(&init_mm.page_table_lock);
 }
 
-/* Return true if pgd is changed, otherwise return false. */
-static bool __meminit free_pud_table(pud_t *pud_start, pgd_t *pgd)
-{
-	pud_t *pud;
-	int i;
-
-	for (i = 0; i < PTRS_PER_PUD; i++) {
-		pud = pud_start + i;
-		if (pud_val(*pud))
-			return false;
-	}
-
-	/* free a pud table */
-	free_pagetable(pgd_page(*pgd), 0);
-	spin_lock(&init_mm.page_table_lock);
-	pgd_clear(pgd);
-	spin_unlock(&init_mm.page_table_lock);
-
-	return true;
-}
-
 static void __meminit
 remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end,
 		 bool direct)
@@ -913,7 +892,6 @@ remove_pagetable(unsigned long start, unsigned long end, bool direct)
 	unsigned long addr;
 	pgd_t *pgd;
 	pud_t *pud;
-	bool pgd_changed = false;
 
 	for (addr = start; addr < end; addr = next) {
 		next = pgd_addr_end(addr, end);
@@ -924,13 +902,8 @@ remove_pagetable(unsigned long start, unsigned long end, bool direct)
 
 		pud = (pud_t *)pgd_page_vaddr(*pgd);
 		remove_pud_table(pud, addr, next, direct);
-		if (free_pud_table(pud, pgd))
-			pgd_changed = true;
 	}
 
-	if (pgd_changed)
-		sync_global_pgds(start, end - 1, 1);
-
 	flush_tlb_all();
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1423602 — [PATCH 12/13] x86/mm/64: Enable vmapped stacks

This allows x86_64 kernels to enable vmapped stacks.  There are a
couple of interesting bits.

First, x86 lazily faults in top-level paging entries for the vmalloc
area.  This won't work if we get a page fault while trying to access
the stack: the CPU will promote it to a double-fault and we'll die.
To avoid this problem, probe the new stack when switching stacks and
forcibly populate the pgd entry for the stack when switching mms.

Second, once we have guard pages around the stack, we'll want to
detect and handle stack overflow.

I didn't enable it on x86_32.  We'd need to rework the double-fault
code a bit and I'm concerned about running out of vmalloc virtual
addresses under some workloads.

This patch, by itself, will behave somewhat erratically when the
stack overflows while RSP is still more than a few tens of bytes
above the bottom of the stack.  Specifically, we'll get #PF and make
it to no_context and an oops without triggering a double-fault, and
no_context doesn't know about stack overflows.  The next patch will
improve that case.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++++++++-
 arch/x86/kernel/traps.c          | 32 ++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c                | 15 +++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0a7b885964ba..b624b24d1dc1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -92,6 +92,7 @@ config X86
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	select HAVE_EBPF_JIT			if X86_64
+	select HAVE_ARCH_VMAP_STACK		if X86_64
 	select HAVE_CC_STACKPROTECTOR
 	select HAVE_CMPXCHG_DOUBLE
 	select HAVE_CMPXCHG_LOCAL
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 8f321a1b03a1..14e4b20f0aaf 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -8,6 +8,28 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
+/* This runs runs on the previous thread's stack. */
+static inline void prepare_switch_to(struct task_struct *prev,
+				     struct task_struct *next)
+{
+#ifdef CONFIG_VMAP_STACK
+	/*
+	 * If we switch to a stack that has a top-level paging entry
+	 * that is not present in the current mm, the resulting #PF will
+	 * will be promoted to a double-fault and we'll panic.  Probe
+	 * the new stack now so that vmalloc_fault can fix up the page
+	 * tables if needed.  This can only happen if we use a stack
+	 * in vmap space.
+	 *
+	 * We assume that the stack is aligned so that it never spans
+	 * more than one top-level paging entry.
+	 *
+	 * To minimize cache pollution, just follow the stack pointer.
+	 */
+	READ_ONCE(*(unsigned char *)next->thread.sp);
+#endif
+}
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR
@@ -39,6 +61,8 @@ do {									\
 	 */								\
 	unsigned long ebx, ecx, edx, esi, edi;				\
 									\
+	prepare_switch_to(prev, next);					\
+									\
 	asm volatile("pushl %%ebp\n\t"		/* save    EBP   */	\
 		     "movl %%esp,%[prev_sp]\n\t"	/* save    ESP   */ \
 		     "movl %[next_sp],%%esp\n\t"	/* restore ESP   */ \
@@ -103,7 +127,9 @@ do {									\
  * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
  * has no effect.
  */
-#define switch_to(prev, next, last) \
+#define switch_to(prev, next, last)					  \
+	prepare_switch_to(prev, next);					  \
+									  \
 	asm volatile(SAVE_CONTEXT					  \
 	     "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */	  \
 	     "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */	  \
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 00f03d82e69a..9cb7ea781176 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -292,12 +292,30 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
 
+#ifdef CONFIG_VMAP_STACK
+static void __noreturn handle_stack_overflow(const char *message,
+					     struct pt_regs *regs,
+					     unsigned long fault_address)
+{
+	printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",
+		 (void *)fault_address, current->stack,
+		 (char *)current->stack + THREAD_SIZE - 1);
+	die(message, regs, 0);
+
+	/* Be absolutely certain we don't return. */
+	panic(message);
+}
+#endif
+
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 {
 	static const char str[] = "double fault";
 	struct task_struct *tsk = current;
+#ifdef CONFIG_VMAP_STACK
+	unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
 	extern unsigned char native_irq_return_iret[];
@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+	/*
+	 * If we overflow the stack into a guard page, the CPU will fail
+	 * to deliver #PF and will send #DF instead.  CR2 will contain
+	 * the linear address of the second fault, which will be in the
+	 * guard page below the bottom of the stack.
+	 */
+	cr2 = read_cr2();
+	if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+		handle_stack_overflow(
+			"kernel stack overflow (double-fault)",
+			regs, cr2);
+#endif
+
 #ifdef CONFIG_DOUBLEFAULT
 	df_debug(regs, error_code);
 #endif
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 5643fd0b1a7d..fbf036ae72ac 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 	unsigned cpu = smp_processor_id();
 
 	if (likely(prev != next)) {
+		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+			/*
+			 * If our current stack is in vmalloc space and isn't
+			 * mapped in the new pgd, we'll double-fault.  Forcibly
+			 * map it.
+			 */
+			unsigned int stack_pgd_index =
+				pgd_index(current_stack_pointer());
+			pgd_t *pgd = next->pgd + stack_pgd_index;
+
+			if (unlikely(pgd_none(*pgd)))
+				set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
+		}
+
 #ifdef CONFIG_SMP
 		this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
 		this_cpu_write(cpu_tlbstate.active_mm, next);
 #endif
+
 		cpumask_set_cpu(cpu, mm_cpumask(next));
 
 		/*
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1423685 — Re: [PATCH 12/13] x86/mm/64: Enable vmapped stacks

On Jun 15, 2016 9:32 PM, "Mika Penttilä" <mika.penttila@nextfour.com> wrote:
>
> Hi,
>

> > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > index 5643fd0b1a7d..fbf036ae72ac 100644
> > --- a/arch/x86/mm/tlb.c
> > +++ b/arch/x86/mm/tlb.c
> > @@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> >       unsigned cpu = smp_processor_id();
> >
> >       if (likely(prev != next)) {
> > +             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > +                     /*
> > +                      * If our current stack is in vmalloc space and isn't
> > +                      * mapped in the new pgd, we'll double-fault.  Forcibly
> > +                      * map it.
> > +                      */
> > +                     unsigned int stack_pgd_index =
> > +                             pgd_index(current_stack_pointer());
>
>
> stack pointer is still the previous task's, current_stack_pointer() returns that, not
> next task's which was intention I guess. Things may happen to work if on same pgd, but at least the
> boot cpu init_task_struct is special.

This is intentional.  When switching processes, we first switch the mm
and then switch the task.  We need to make sure that the prev stack is
mapped in the new mm or we'll double-fault and die after switching the
mm which still trying to execute on the old stack.

The change to switch_to makes sure that the new stack is mapped.

--Andy

[toc] | [prev] | [next] | [standalone]

#1424040 — Re: [kernel-hardening] Re: [PATCH 12/13] x86/mm/64: Enable vmapped stacks

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-06-15 at 22:33 -0700, Andy Lutomirski wrote:
> 
> > > +++ b/arch/x86/mm/tlb.c
> > > @@ -77,10 +77,25 @@ void switch_mm_irqs_off(struct mm_struct
> > > *prev, struct mm_struct *next,
> > >       unsigned cpu = smp_processor_id();
> > > 
> > >       if (likely(prev != next)) {
> > > +             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > +                     /*
> > > +                      * If our current stack is in vmalloc space
> > > and isn't
> > > +                      * mapped in the new pgd, we'll double-
> > > fault.  Forcibly
> > > +                      * map it.
> > > +                      */
> > > +                     unsigned int stack_pgd_index =
> > > +                             pgd_index(current_stack_pointer());
> > 
> > stack pointer is still the previous task's, current_stack_pointer()
> > returns that, not
> > next task's which was intention I guess. Things may happen to work
> > if on same pgd, but at least the
> > boot cpu init_task_struct is special.
> This is intentional.  When switching processes, we first switch the
> mm
> and then switch the task.  We need to make sure that the prev stack
> is
> mapped in the new mm or we'll double-fault and die after switching
> the
> mm which still trying to execute on the old stack.
> 
> The change to switch_to makes sure that the new stack is mapped.
> 

On a HARDENED_USERCOPY tangential note: by not allowing
copy_to/from_user access to vmalloc memory by default,
with exception of the stack, a task will only be able
to copy_to/from_user from its own stack, not another task's
stack, at least using the kernel virtual address the
kernel uses to access that stack.

This can be accomplished by simply not adding any vmalloc
checking code to the current HARDENED_USERCOPY patch set :)

-- 
All rights reversed

[toc] | [prev] | [next] | [standalone]

#1423603 — [PATCH 03/13] x86/cpa: Warn if kernel_unmap_pages_in_pgd is used inappropriately

It's currently only used in the EFI code, which is safe AFAICT.
Warn if anyone tries to use it on the normal kernel pgd.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/pageattr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 6a8026918bf6..e9b9c5cedbb8 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1996,6 +1996,9 @@ out:
 void kernel_unmap_pages_in_pgd(pgd_t *root, unsigned long address,
 			       unsigned numpages)
 {
+	/* Unmapping kernel entries from init_mm's pgd is not allowed. */
+	WARN_ON(root == init_mm.pgd);
+
 	unmap_pgd_range(root, address, address + (numpages << PAGE_SHIFT));
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1423605 — [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack before do_exit

If we call do_exit with a clean stack, we greatly reduce the risk of
recursive oopses due to stack overflow in do_exit, and we allow
do_exit to work even if we OOPS from an IST stack.  The latter gives
us a much better chance of surviving long enough after we detect a
stack overflow to write out our logs.

I intentionally separated this from the preceding patch that
disables do_exit-on-OOPS on IST stacks.  This way, if we need to
revert this patch, we still end up in an acceptable state wrt stack
overflow handling.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/entry_32.S   | 11 +++++++++++
 arch/x86/entry/entry_64.S   | 11 +++++++++++
 arch/x86/kernel/dumpstack.c | 13 +++++++++----
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 983e5d3a0d27..1499db695a88 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1153,3 +1153,14 @@ ENTRY(async_page_fault)
 	jmp	error_code
 END(async_page_fault)
 #endif
+
+ENTRY(rewind_stack_do_exit)
+	/* Prevent any naive code from trying to unwind to our caller. */
+	xorl	%ebp, %ebp
+
+	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esi
+	leal	-TOP_OF_KERNEL_STACK_PADDING-PT_OLDSS(%esi), %esp
+
+	call	do_exit
+1:	jmp 1b
+END(rewind_stack_do_exit)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 9ee0da1807ed..394cad73e890 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1423,3 +1423,14 @@ ENTRY(ignore_sysret)
 	mov	$-ENOSYS, %eax
 	sysret
 END(ignore_sysret)
+
+ENTRY(rewind_stack_do_exit)
+	/* Prevent any naive code from trying to unwind to our caller. */
+	xorl	%ebp, %ebp
+
+	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rax
+	leaq	-TOP_OF_KERNEL_STACK_PADDING-SS(%rax), %rsp
+
+	call	do_exit
+1:	jmp 1b
+END(rewind_stack_do_exit)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 36effb39c9c9..d4d085e27d04 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -228,6 +228,8 @@ unsigned long oops_begin(void)
 EXPORT_SYMBOL_GPL(oops_begin);
 NOKPROBE_SYMBOL(oops_begin);
 
+extern void __noreturn rewind_stack_do_exit(int signr);
+
 void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 {
 	if (regs && kexec_should_crash(current))
@@ -247,12 +249,15 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 		return;
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
-	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
-	     & ~(THREAD_SIZE - 1)) != 0)
-		panic("Fatal exception on special stack");
 	if (panic_on_oops)
 		panic("Fatal exception");
-	do_exit(signr);
+
+	/*
+	 * We're not going to return, but we might be on an IST stack or
+	 * have very little stack space left.  Rewind the stack and kill
+	 * the task.
+	 */
+	rewind_stack_do_exit(signr);
 }
 NOKPROBE_SYMBOL(oops_end);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1424321 — Re: [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack before do_exit

On Wed, Jun 15, 2016 at 05:28:30PM -0700, Andy Lutomirski wrote:
> If we call do_exit with a clean stack, we greatly reduce the risk of
> recursive oopses due to stack overflow in do_exit, and we allow
> do_exit to work even if we OOPS from an IST stack.  The latter gives
> us a much better chance of surviving long enough after we detect a
> stack overflow to write out our logs.
> 
> I intentionally separated this from the preceding patch that
> disables do_exit-on-OOPS on IST stacks.  This way, if we need to
> revert this patch, we still end up in an acceptable state wrt stack
> overflow handling.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/entry_32.S   | 11 +++++++++++
>  arch/x86/entry/entry_64.S   | 11 +++++++++++
>  arch/x86/kernel/dumpstack.c | 13 +++++++++----
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 983e5d3a0d27..1499db695a88 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -1153,3 +1153,14 @@ ENTRY(async_page_fault)
>  	jmp	error_code
>  END(async_page_fault)
>  #endif
> +
> +ENTRY(rewind_stack_do_exit)
> +	/* Prevent any naive code from trying to unwind to our caller. */
> +	xorl	%ebp, %ebp
> +
> +	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esi
> +	leal	-TOP_OF_KERNEL_STACK_PADDING-PT_OLDSS(%esi), %esp
> +
> +	call	do_exit
> +1:	jmp 1b
> +END(rewind_stack_do_exit)
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 9ee0da1807ed..394cad73e890 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1423,3 +1423,14 @@ ENTRY(ignore_sysret)
>  	mov	$-ENOSYS, %eax
>  	sysret
>  END(ignore_sysret)
> +
> +ENTRY(rewind_stack_do_exit)
> +	/* Prevent any naive code from trying to unwind to our caller. */
> +	xorl	%ebp, %ebp
> +
> +	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rax
> +	leaq	-TOP_OF_KERNEL_STACK_PADDING-SS(%rax), %rsp

I think this should be:

	leaq	-TOP_OF_KERNEL_STACK_PADDING-SIZEOF_PTREGS, %rsp

That way when it calls do_exit(), the stack frame will be placed at the
conventional spot where a smart unwinder would expect to find it.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1424327 — Re: [PATCH 08/13] x86/dumpstack: When OOPSing, rewind the stack before do_exit

On Thu, Jun 16, 2016 at 10:50 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Jun 15, 2016 at 05:28:30PM -0700, Andy Lutomirski wrote:
>> If we call do_exit with a clean stack, we greatly reduce the risk of
>> recursive oopses due to stack overflow in do_exit, and we allow
>> do_exit to work even if we OOPS from an IST stack.  The latter gives
>> us a much better chance of surviving long enough after we detect a
>> stack overflow to write out our logs.
>>
>> I intentionally separated this from the preceding patch that
>> disables do_exit-on-OOPS on IST stacks.  This way, if we need to
>> revert this patch, we still end up in an acceptable state wrt stack
>> overflow handling.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/entry/entry_32.S   | 11 +++++++++++
>>  arch/x86/entry/entry_64.S   | 11 +++++++++++
>>  arch/x86/kernel/dumpstack.c | 13 +++++++++----
>>  3 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>> index 983e5d3a0d27..1499db695a88 100644
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -1153,3 +1153,14 @@ ENTRY(async_page_fault)
>>       jmp     error_code
>>  END(async_page_fault)
>>  #endif
>> +
>> +ENTRY(rewind_stack_do_exit)
>> +     /* Prevent any naive code from trying to unwind to our caller. */
>> +     xorl    %ebp, %ebp
>> +
>> +     movl    PER_CPU_VAR(cpu_current_top_of_stack), %esi
>> +     leal    -TOP_OF_KERNEL_STACK_PADDING-PT_OLDSS(%esi), %esp
>> +
>> +     call    do_exit
>> +1:   jmp 1b
>> +END(rewind_stack_do_exit)
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index 9ee0da1807ed..394cad73e890 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -1423,3 +1423,14 @@ ENTRY(ignore_sysret)
>>       mov     $-ENOSYS, %eax
>>       sysret
>>  END(ignore_sysret)
>> +
>> +ENTRY(rewind_stack_do_exit)
>> +     /* Prevent any naive code from trying to unwind to our caller. */
>> +     xorl    %ebp, %ebp
>> +
>> +     movq    PER_CPU_VAR(cpu_current_top_of_stack), %rax
>> +     leaq    -TOP_OF_KERNEL_STACK_PADDING-SS(%rax), %rsp
>
> I think this should be:
>
>         leaq    -TOP_OF_KERNEL_STACK_PADDING-SIZEOF_PTREGS, %rsp
>
> That way when it calls do_exit(), the stack frame will be placed at the
> conventional spot where a smart unwinder would expect to find it.

Whoops!

--Andy

[toc] | [prev] | [next] | [standalone]

#1423606 — [PATCH 02/13] x86/cpa: In populate_pgd, don't set the pgd entry until it's populated

This avoids pointless races in which another CPU or task might see a
partially populated global pgd entry.  These races should normally
be harmless, but, if another CPU propagates the entry via
vmalloc_fault and then populate_pgd fails (due to memory allocation
failure, for example), this prevents a use-after-free of the pgd
entry.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/pageattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 7a1f7bbf4105..6a8026918bf6 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1113,7 +1113,9 @@ static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
 
 	ret = populate_pud(cpa, addr, pgd_entry, pgprot);
 	if (ret < 0) {
-		unmap_pgd_range(cpa->pgd, addr,
+		if (pud)
+			free_page((unsigned long)pud);
+		unmap_pud_range(pgd_entry, addr,
 				addr + (cpa->numpages << PAGE_SHIFT));
 		return ret;
 	}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1423607 — [PATCH 06/13] fork: Add generic vmalloced stack support

If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with
vmalloc_node.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/Kconfig  | 12 ++++++++++++
 kernel/fork.c | 45 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index d794384a0404..1acd262036b0 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -658,4 +658,16 @@ config ARCH_NO_COHERENT_DMA_MMAP
 config CPU_NO_EFFICIENT_FFS
 	def_bool n
 
+config HAVE_ARCH_VMAP_STACK
+       def_bool n
+
+config VMAP_STACK
+	bool "Use a virtually-mapped stack"
+	depends on HAVE_ARCH_VMAP_STACK
+	---help---
+	  Enable this if you want the use virtually-mapped kernel stacks
+	  with guard pages.  This causes kernel stack overflows to be
+	  caught immediately rather than causing difficult-to-diagnose
+	  corruption.
+
 source "kernel/gcov/Kconfig"
diff --git a/kernel/fork.c b/kernel/fork.c
index 59e52f2120a3..37234fa0ba9b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -158,19 +158,30 @@ void __weak arch_release_thread_info(struct thread_info *ti)
  * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
  * kmemcache based allocator.
  */
-# if THREAD_SIZE >= PAGE_SIZE
+# if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
 static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
 						  int node)
 {
+#ifdef CONFIG_VMAP_STACK
+	return __vmalloc_node_range(
+		THREAD_SIZE, THREAD_SIZE, VMALLOC_START, VMALLOC_END,
+		GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL,
+		0, node, __builtin_return_address(0));
+#else
 	struct page *page = alloc_kmem_pages_node(node, THREADINFO_GFP,
 						  THREAD_SIZE_ORDER);
 
 	return page ? page_address(page) : NULL;
+#endif
 }
 
 static inline void free_thread_info(struct thread_info *ti)
 {
+#ifdef CONFIG_VMAP_STACK
+	vfree(ti);
+#else
 	free_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
+#endif
 }
 # else
 static struct kmem_cache *thread_info_cache;
@@ -215,15 +226,33 @@ static struct kmem_cache *mm_cachep;
 
 static void account_kernel_stack(struct thread_info *ti, int account)
 {
-	struct zone *zone = page_zone(virt_to_page(ti));
+	struct zone *zone;
+
+	if (IS_ENABLED(CONFIG_VMAP_STACK) && !virt_addr_valid(ti)) {
+		int i;
+		struct vm_struct *vm = find_vm_area(ti);
 
-	mod_zone_page_state(zone, NR_KERNEL_STACK,
-			    THREAD_SIZE / PAGE_SIZE * account);
+		WARN_ON_ONCE(vm->nr_pages != THREAD_SIZE / PAGE_SIZE);
 
-	/* All stack pages belong to the same memcg. */
-	memcg_kmem_update_page_stat(
-		virt_to_page(ti), MEMCG_KERNEL_STACK,
-		account * (THREAD_SIZE / PAGE_SIZE));
+		for (i = 0; i < THREAD_SIZE / PAGE_SIZE; i++) {
+			mod_zone_page_state(page_zone(vm->pages[i]),
+					    1, account);
+		}
+
+		/* All stack pages belong to the same memcg. */
+		memcg_kmem_update_page_stat(
+			vm->pages[0], MEMCG_KERNEL_STACK,
+			account * (THREAD_SIZE / PAGE_SIZE));
+	} else {
+		zone = page_zone(virt_to_page(ti));
+		mod_zone_page_state(zone, NR_KERNEL_STACK,
+				    THREAD_SIZE / PAGE_SIZE * account);
+
+		/* All stack pages belong to the same memcg. */
+		memcg_kmem_update_page_stat(
+			virt_to_page(ti), MEMCG_KERNEL_STACK,
+			account * (THREAD_SIZE / PAGE_SIZE));
+	}
 }
 
 void free_task(struct task_struct *tsk)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1424294 — Re: [PATCH 06/13] fork: Add generic vmalloced stack support

On Wed, Jun 15, 2016 at 5:28 PM, Andy Lutomirski <luto@kernel.org> wrote:
> If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with
> vmalloc_node.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/Kconfig  | 12 ++++++++++++
>  kernel/fork.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index d794384a0404..1acd262036b0 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -658,4 +658,16 @@ config ARCH_NO_COHERENT_DMA_MMAP
>  config CPU_NO_EFFICIENT_FFS
>         def_bool n
>
> +config HAVE_ARCH_VMAP_STACK
> +       def_bool n

In the style of HAVE_ARCH_SECCOMP_FILTER, can you detail all the
things an architecture needs to do to correctly support a vmap stack?
This will help with arch porting.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1424305 — Re: [PATCH 06/13] fork: Add generic vmalloced stack support

On Thu, Jun 16, 2016 at 10:25 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 15, 2016 at 5:28 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with
>> vmalloc_node.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/Kconfig  | 12 ++++++++++++
>>  kernel/fork.c | 45 +++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index d794384a0404..1acd262036b0 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -658,4 +658,16 @@ config ARCH_NO_COHERENT_DMA_MMAP
>>  config CPU_NO_EFFICIENT_FFS
>>         def_bool n
>>
>> +config HAVE_ARCH_VMAP_STACK
>> +       def_bool n
>
> In the style of HAVE_ARCH_SECCOMP_FILTER, can you detail all the
> things an architecture needs to do to correctly support a vmap stack?
> This will help with arch porting.

Done.

[toc] | [prev] | [next] | [standalone]

#1423608 — [PATCH 07/13] x86/die: Don't try to recover from an OOPS on a non-default stack

It's not going to work, because the scheduler will explode if we try
to schedule when running on an IST stack or similar.

This will matter when we let kernel stack overflows (which are #DF)
call die().

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/dumpstack.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 2bb25c3fe2e8..36effb39c9c9 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -247,6 +247,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 		return;
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
+	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
+	     & ~(THREAD_SIZE - 1)) != 0)
+		panic("Fatal exception on special stack");
 	if (panic_on_oops)
 		panic("Fatal exception");
 	do_exit(signr);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1423611 — [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

If we overflow the stack, print_context_stack will abort.  Detect
this case and rewind back into the valid part of the stack so that
we can trace it.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/dumpstack.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index d4d085e27d04..400a2e17c1d1 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
 {
 	struct stack_frame *frame = (struct stack_frame *)bp;
 
+	/*
+	 * If we overflowed the stack into a guard page, jump back to the
+	 * bottom of the usable stack.
+	 */
+	if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
+		stack = (unsigned long *)tinfo + 1;
+
 	while (valid_stack_ptr(tinfo, stack, sizeof(*stack), end)) {
 		unsigned long addr;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1424335 — Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

On Wed, Jun 15, 2016 at 05:28:32PM -0700, Andy Lutomirski wrote:
> If we overflow the stack, print_context_stack will abort.  Detect
> this case and rewind back into the valid part of the stack so that
> we can trace it.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/dumpstack.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index d4d085e27d04..400a2e17c1d1 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
>  {
>  	struct stack_frame *frame = (struct stack_frame *)bp;
>  
> +	/*
> +	 * If we overflowed the stack into a guard page, jump back to the
> +	 * bottom of the usable stack.
> +	 */
> +	if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
> +		stack = (unsigned long *)tinfo + 1;

That will start walking the stack in the middle of the thread_info
struct.

I think you meant:

		stack = (unsigned long *)(tinfo + 1)

However, thread_info will have been overwritten anyway.  So maybe it
should just be:

		stack = tinfo;

(Though that still wouldn't quite work because the valid_stack_ptr()
check would fail...)

> +
>  	while (valid_stack_ptr(tinfo, stack, sizeof(*stack), end)) {
>  		unsigned long addr;

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1424339 — Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

On Thu, Jun 16, 2016 at 11:16 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Jun 15, 2016 at 05:28:32PM -0700, Andy Lutomirski wrote:
>> If we overflow the stack, print_context_stack will abort.  Detect
>> this case and rewind back into the valid part of the stack so that
>> we can trace it.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/kernel/dumpstack.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
>> index d4d085e27d04..400a2e17c1d1 100644
>> --- a/arch/x86/kernel/dumpstack.c
>> +++ b/arch/x86/kernel/dumpstack.c
>> @@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
>>  {
>>       struct stack_frame *frame = (struct stack_frame *)bp;
>>
>> +     /*
>> +      * If we overflowed the stack into a guard page, jump back to the
>> +      * bottom of the usable stack.
>> +      */
>> +     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
>> +             stack = (unsigned long *)tinfo + 1;
>
> That will start walking the stack in the middle of the thread_info
> struct.
>
> I think you meant:
>
>                 stack = (unsigned long *)(tinfo + 1)
>
> However, thread_info will have been overwritten anyway.  So maybe it
> should just be:
>
>                 stack = tinfo;
>
> (Though that still wouldn't quite work because the valid_stack_ptr()
> check would fail...)

I did mean what I wrote, because I wanted to start at the bottom of
the validly allocated area.  IOW I wanted to do the minimum possible
backward jump to make the code display something.

Eventually I want to make thread_info empty, in which case the
distinction won't matter so much.

--Andy

[toc] | [prev] | [next] | [standalone]

#1424341 — Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

On Thu, Jun 16, 2016 at 11:33 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jun 16, 2016 at 11:22:14AM -0700, Andy Lutomirski wrote:
>> On Thu, Jun 16, 2016 at 11:16 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Wed, Jun 15, 2016 at 05:28:32PM -0700, Andy Lutomirski wrote:
>> >> If we overflow the stack, print_context_stack will abort.  Detect
>> >> this case and rewind back into the valid part of the stack so that
>> >> we can trace it.
>> >>
>> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> >> ---
>> >>  arch/x86/kernel/dumpstack.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
>> >> index d4d085e27d04..400a2e17c1d1 100644
>> >> --- a/arch/x86/kernel/dumpstack.c
>> >> +++ b/arch/x86/kernel/dumpstack.c
>> >> @@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
>> >>  {
>> >>       struct stack_frame *frame = (struct stack_frame *)bp;
>> >>
>> >> +     /*
>> >> +      * If we overflowed the stack into a guard page, jump back to the
>> >> +      * bottom of the usable stack.
>> >> +      */
>> >> +     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
>> >> +             stack = (unsigned long *)tinfo + 1;
>> >
>> > That will start walking the stack in the middle of the thread_info
>> > struct.
>> >
>> > I think you meant:
>> >
>> >                 stack = (unsigned long *)(tinfo + 1)
>> >
>> > However, thread_info will have been overwritten anyway.  So maybe it
>> > should just be:
>> >
>> >                 stack = tinfo;
>> >
>> > (Though that still wouldn't quite work because the valid_stack_ptr()
>> > check would fail...)
>>
>> I did mean what I wrote, because I wanted to start at the bottom of
>> the validly allocated area.  IOW I wanted to do the minimum possible
>> backward jump to make the code display something.
>
> But why the "+ 1"?  Is that a hack to make it pass the valid_stack_ptr()
> check?

Yes.

But hmm.  Maybe the right fix is to drop the + 1 and to change the
last line of valid_stck_ptr from:

    return p > t && p < t + THREAD_SIZE - size;

to:

    return p >= t && p < t + THREAD_SIZE - size;

The current definition of valid_stack_ptr is certainly nonsensical.
It should either be p >= t or p >= t + 1.

--Andy

[toc] | [prev] | [next] | [standalone]

#1424349 — Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

On Thu, Jun 16, 2016 at 11:37:07AM -0700, Andy Lutomirski wrote:
> On Thu, Jun 16, 2016 at 11:33 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Jun 16, 2016 at 11:22:14AM -0700, Andy Lutomirski wrote:
> >> On Thu, Jun 16, 2016 at 11:16 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Wed, Jun 15, 2016 at 05:28:32PM -0700, Andy Lutomirski wrote:
> >> >> If we overflow the stack, print_context_stack will abort.  Detect
> >> >> this case and rewind back into the valid part of the stack so that
> >> >> we can trace it.
> >> >>
> >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> >> ---
> >> >>  arch/x86/kernel/dumpstack.c | 7 +++++++
> >> >>  1 file changed, 7 insertions(+)
> >> >>
> >> >> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> >> >> index d4d085e27d04..400a2e17c1d1 100644
> >> >> --- a/arch/x86/kernel/dumpstack.c
> >> >> +++ b/arch/x86/kernel/dumpstack.c
> >> >> @@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
> >> >>  {
> >> >>       struct stack_frame *frame = (struct stack_frame *)bp;
> >> >>
> >> >> +     /*
> >> >> +      * If we overflowed the stack into a guard page, jump back to the
> >> >> +      * bottom of the usable stack.
> >> >> +      */
> >> >> +     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
> >> >> +             stack = (unsigned long *)tinfo + 1;
> >> >
> >> > That will start walking the stack in the middle of the thread_info
> >> > struct.
> >> >
> >> > I think you meant:
> >> >
> >> >                 stack = (unsigned long *)(tinfo + 1)
> >> >
> >> > However, thread_info will have been overwritten anyway.  So maybe it
> >> > should just be:
> >> >
> >> >                 stack = tinfo;
> >> >
> >> > (Though that still wouldn't quite work because the valid_stack_ptr()
> >> > check would fail...)
> >>
> >> I did mean what I wrote, because I wanted to start at the bottom of
> >> the validly allocated area.  IOW I wanted to do the minimum possible
> >> backward jump to make the code display something.
> >
> > But why the "+ 1"?  Is that a hack to make it pass the valid_stack_ptr()
> > check?
> 
> Yes.
> 
> But hmm.  Maybe the right fix is to drop the + 1 and to change the
> last line of valid_stck_ptr from:
> 
>     return p > t && p < t + THREAD_SIZE - size;
> 
> to:
> 
>     return p >= t && p < t + THREAD_SIZE - size;

Yeah, I think that would be much better.  Then it won't skip the first
value on the page.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1424343 — Re: [PATCH 10/13] x86/dumpstack: Try harder to get a call trace on stack overflow

On Thu, Jun 16, 2016 at 11:22:14AM -0700, Andy Lutomirski wrote:
> On Thu, Jun 16, 2016 at 11:16 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Wed, Jun 15, 2016 at 05:28:32PM -0700, Andy Lutomirski wrote:
> >> If we overflow the stack, print_context_stack will abort.  Detect
> >> this case and rewind back into the valid part of the stack so that
> >> we can trace it.
> >>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> ---
> >>  arch/x86/kernel/dumpstack.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> >> index d4d085e27d04..400a2e17c1d1 100644
> >> --- a/arch/x86/kernel/dumpstack.c
> >> +++ b/arch/x86/kernel/dumpstack.c
> >> @@ -100,6 +100,13 @@ print_context_stack(struct thread_info *tinfo,
> >>  {
> >>       struct stack_frame *frame = (struct stack_frame *)bp;
> >>
> >> +     /*
> >> +      * If we overflowed the stack into a guard page, jump back to the
> >> +      * bottom of the usable stack.
> >> +      */
> >> +     if ((unsigned long)tinfo - (unsigned long)stack < PAGE_SIZE)
> >> +             stack = (unsigned long *)tinfo + 1;
> >
> > That will start walking the stack in the middle of the thread_info
> > struct.
> >
> > I think you meant:
> >
> >                 stack = (unsigned long *)(tinfo + 1)
> >
> > However, thread_info will have been overwritten anyway.  So maybe it
> > should just be:
> >
> >                 stack = tinfo;
> >
> > (Though that still wouldn't quite work because the valid_stack_ptr()
> > check would fail...)
> 
> I did mean what I wrote, because I wanted to start at the bottom of
> the validly allocated area.  IOW I wanted to do the minimum possible
> backward jump to make the code display something.

But why the "+ 1"?  Is that a hack to make it pass the valid_stack_ptr()
check?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web