x86/dumpstack/64: Don't evaluate exception stacks before setup
diff mbox series

Message ID alpine.DEB.2.21.1910231950590.1852@nanos.tec.linutronix.de
State New
Headers show
Series
  • x86/dumpstack/64: Don't evaluate exception stacks before setup
Related show

Commit Message

Thomas Gleixner Oct. 23, 2019, 6:05 p.m. UTC
Cyrill reported the following crash:

  BUG: unable to handle page fault for address: 0000000000001ff0
  #PF: supervisor read access in kernel mode
  RIP: 0010:get_stack_info+0xb3/0x148

It turns out that if the stack tracer is invoked before the exception stack
mappings are initialized in_exception_stack() can erroneously classify an
invalid address as an address inside of an exception stack:

    begin = this_cpu_read(cea_exception_stacks);  <- 0
    end = begin + sizeof(exception stacks);

i.e. any address between 0 and end will be considered as exception stack
address and the subsequent code will then try to derefence the resulting
stack frame at a non mapped address.

 end = begin + (unsigned long)ep->size;
     ==> end = 0x2000

 regs = (struct pt_regs *)end - 1;
     ==> regs = 0x2000 - sizeof(struct pt_regs *) = 0x1ff0

 info->next_sp   = (unsigned long *)regs->sp;
     ==> Crashes due to accessing 0x1ff0

Prevent this by checking the validity of the cea_exception_stack base
address and bailing out if it is zero.

Fixes: afcd21dad88b ("x86/dumpstack/64: Use cpu_entry_area instead of orig_ist")
Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/dumpstack_64.c |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Matthew Wilcox Oct. 23, 2019, 6:31 p.m. UTC | #1
On Wed, Oct 23, 2019 at 08:05:49PM +0200, Thomas Gleixner wrote:
> Prevent this by checking the validity of the cea_exception_stack base
> address and bailing out if it is zero.

Could also initialise cea_exception_stack to -1?  That would lead to it
being caught by ...

>  	end = begin + sizeof(struct cea_exception_stacks);
>  	/* Bail if @stack is outside the exception stack area. */
>  	if (stk < begin || stk >= end)

this existing check.
Cyrill Gorcunov Oct. 23, 2019, 6:43 p.m. UTC | #2
On Wed, Oct 23, 2019 at 11:31:40AM -0700, Matthew Wilcox wrote:
> On Wed, Oct 23, 2019 at 08:05:49PM +0200, Thomas Gleixner wrote:
> > Prevent this by checking the validity of the cea_exception_stack base
> > address and bailing out if it is zero.
> 
> Could also initialise cea_exception_stack to -1?  That would lead to it
> being caught by ...
> 
> >  	end = begin + sizeof(struct cea_exception_stacks);
> >  	/* Bail if @stack is outside the exception stack area. */
> >  	if (stk < begin || stk >= end)
> 
> this existing check.

As to me this would be a hack and fragile :/ In turn the current explicit
test Thomas made is a way more readable.
Josh Poimboeuf Oct. 23, 2019, 7:17 p.m. UTC | #3
On Wed, Oct 23, 2019 at 08:05:49PM +0200, Thomas Gleixner wrote:
> Cyrill reported the following crash:
> 
>   BUG: unable to handle page fault for address: 0000000000001ff0
>   #PF: supervisor read access in kernel mode
>   RIP: 0010:get_stack_info+0xb3/0x148
> 
> It turns out that if the stack tracer is invoked before the exception stack
> mappings are initialized in_exception_stack() can erroneously classify an
> invalid address as an address inside of an exception stack:
> 
>     begin = this_cpu_read(cea_exception_stacks);  <- 0
>     end = begin + sizeof(exception stacks);
> 
> i.e. any address between 0 and end will be considered as exception stack
> address and the subsequent code will then try to derefence the resulting
> stack frame at a non mapped address.
> 
>  end = begin + (unsigned long)ep->size;
>      ==> end = 0x2000
> 
>  regs = (struct pt_regs *)end - 1;
>      ==> regs = 0x2000 - sizeof(struct pt_regs *) = 0x1ff0
> 
>  info->next_sp   = (unsigned long *)regs->sp;
>      ==> Crashes due to accessing 0x1ff0
> 
> Prevent this by checking the validity of the cea_exception_stack base
> address and bailing out if it is zero.
> 
> Fixes: afcd21dad88b ("x86/dumpstack/64: Use cpu_entry_area instead of orig_ist")
> Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: stable@vger.kernel.org

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Thomas Gleixner Oct. 23, 2019, 9:27 p.m. UTC | #4
On Wed, 23 Oct 2019, Matthew Wilcox wrote:

> On Wed, Oct 23, 2019 at 08:05:49PM +0200, Thomas Gleixner wrote:
> > Prevent this by checking the validity of the cea_exception_stack base
> > address and bailing out if it is zero.
> 
> Could also initialise cea_exception_stack to -1?  That would lead to it
> being caught by ...
> 
> >  	end = begin + sizeof(struct cea_exception_stacks);
> >  	/* Bail if @stack is outside the exception stack area. */
> >  	if (stk < begin || stk >= end)
> 
> this existing check.

Yes thought about that, but then decided to do it in a readable way :)

Patch
diff mbox series

--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -94,6 +94,13 @@  static bool in_exception_stack(unsigned
 	BUILD_BUG_ON(N_EXCEPTION_STACKS != 6);
 
 	begin = (unsigned long)__this_cpu_read(cea_exception_stacks);
+	/*
+	 * Handle the case where stack trace is collected _before_
+	 * cea_exception_stacks had been initialized.
+	 */
+	if (!begin)
+		return false;
+
 	end = begin + sizeof(struct cea_exception_stacks);
 	/* Bail if @stack is outside the exception stack area. */
 	if (stk < begin || stk >= end)