linux-kernel - Re: [PATCH 02/21] x86/unwinder: Handle stack overflows more gracefully

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20171127173604.eff4ta576mpxjhjg@pd.tnic>
Date:   Mon, 27 Nov 2017 18:36:04 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...capital.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 02/21] x86/unwinder: Handle stack overflows more
 gracefully

On Mon, Nov 27, 2017 at 11:45:10AM +0100, Ingo Molnar wrote:
> From: Josh Poimboeuf <jpoimboe@...hat.com>
> 
> There are at least two unwinder bugs hindering the debugging of
> stack-overflow crashes:
> 
> - It doesn't deal gracefully with the case where the stack overflows and
>   the stack pointer itself isn't on a valid stack but the
>   to-be-dereferenced data *is*.
> 
> - The ORC oops dump code doesn't know how to print partial pt_regs, for the
>   case where if we get an interrupt/exception in *early* entry code
>   before the full pt_regs have been saved.
> 
> Fix both issues.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Borislav Petkov <bpetkov@...e.de>
> Cc: Brian Gerst <brgerst@...il.com>
> Cc: Dave Hansen <dave.hansen@...el.com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Link: http://lkml.kernel.org/r/20171126024031.uxi4numpbjm5rlbr@treble
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> ---
>  arch/x86/include/asm/kdebug.h |  1 +
>  arch/x86/include/asm/unwind.h |  7 ++++
>  arch/x86/kernel/dumpstack.c   | 32 ++++++++++++++++---
>  arch/x86/kernel/process_64.c  | 11 +++----
>  arch/x86/kernel/stacktrace.c  |  2 +-
>  arch/x86/kernel/unwind_orc.c  | 74 +++++++++++++++----------------------------
>  6 files changed, 66 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
> index f86a8caa561e..395c9631e000 100644
> --- a/arch/x86/include/asm/kdebug.h
> +++ b/arch/x86/include/asm/kdebug.h
> @@ -26,6 +26,7 @@ extern void die(const char *, struct pt_regs *,long);
>  extern int __must_check __die(const char *, struct pt_regs *, long);
>  extern void show_stack_regs(struct pt_regs *regs);
>  extern void __show_regs(struct pt_regs *regs, int all);
> +extern void show_iret_regs(struct pt_regs *regs);
>  extern unsigned long oops_begin(void);
>  extern void oops_end(unsigned long, struct pt_regs *, int signr);
>  
> diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
> index e9cc6fe1fc6f..5be2fb23825a 100644
> --- a/arch/x86/include/asm/unwind.h
> +++ b/arch/x86/include/asm/unwind.h
> @@ -7,6 +7,9 @@
>  #include <asm/ptrace.h>
>  #include <asm/stacktrace.h>
>  
> +#define IRET_FRAME_OFFSET (offsetof(struct pt_regs, ip))
> +#define IRET_FRAME_SIZE   (sizeof(struct pt_regs) - IRET_FRAME_OFFSET)
> +
>  struct unwind_state {
>  	struct stack_info stack_info;
>  	unsigned long stack_mask;
> @@ -52,6 +55,10 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
>  }
>  
>  #if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER)
> +/*
> + * WARNING: The entire pt_regs may not be safe to dereference.  In some cases,
> + * only the iret registers are accessible.  Use with caution!
	   ^^^^^^^^^^^^^^^^^^

You mean the interrupt stack frame here, right?

> + */
>  static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
>  {
>  	if (unwind_done(state))
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index f13b4c00a5de..fc918744ad7d 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -50,6 +50,28 @@ static void printk_stack_address(unsigned long address, int reliable,
>  	printk("%s %s%pB\n", log_lvl, reliable ? "" : "? ", (void *)address);
>  }
>  
> +void show_iret_regs(struct pt_regs *regs)
> +{
> +	printk(KERN_DEFAULT "RIP: %04x:%pS\n", (int)regs->cs, (void *)regs->ip);
> +	printk(KERN_DEFAULT "RSP: %04x:%016lx EFLAGS: %08lx", (int)regs->ss,
> +		regs->sp, regs->flags);
> +}
> +
> +static void show_regs_safe(struct stack_info *info, struct pt_regs *regs)
> +{
> +	if (on_stack(info, regs, sizeof(*regs)))
> +		__show_regs(regs, 0);
> +	else if (on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
> +			  IRET_FRAME_SIZE)) {
> +		/*
> +		 * When an interrupt or exception occurs in entry code, the
> +		 * full pt_regs might not have been saved yet.  In that case
> +		 * just print the iret return frame.

Right, it is the interrupt stack frame. But "iret frame" is shorter so
let's stick to that :)

...

> @@ -283,42 +276,32 @@ static bool deref_stack_reg(struct unwind_state *state, unsigned long addr,
>  	return true;
>  }
>  
> -#define REGS_SIZE (sizeof(struct pt_regs))
> -#define SP_OFFSET (offsetof(struct pt_regs, sp))
> -#define IRET_REGS_SIZE (REGS_SIZE - offsetof(struct pt_regs, ip))
> -#define IRET_SP_OFFSET (SP_OFFSET - offsetof(struct pt_regs, ip))
> -
>  static bool deref_stack_regs(struct unwind_state *state, unsigned long addr,
> -			     unsigned long *ip, unsigned long *sp, bool full)
> +			     unsigned long *ip, unsigned long *sp)
>  {
> -	size_t regs_size = full ? REGS_SIZE : IRET_REGS_SIZE;
> -	size_t sp_offset = full ? SP_OFFSET : IRET_SP_OFFSET;
> -	struct pt_regs *regs = (struct pt_regs *)(addr + regs_size - REGS_SIZE);
> -
> -	if (IS_ENABLED(CONFIG_X86_64)) {
> -		if (!stack_access_ok(state, addr, regs_size))
> -			return false;
> -
> -		*ip = regs->ip;
> -		*sp = regs->sp;
> +	struct pt_regs *regs = (struct pt_regs *)addr;
>  
> -		return true;
> -	}
> +	/* x86-32 support will be more complicated due to the &regs->sp hack */
> +	BUILD_BUG_ON(IS_ENABLED(CONFIG_X86_32));
>  
> -	if (!stack_access_ok(state, addr, sp_offset))
> +	if (!stack_access_ok(state, addr, sizeof(struct pt_regs)))
>  		return false;
>  
>  	*ip = regs->ip;
> +	*sp = regs->sp;
> +	return true;
> +}
>  
> -	if (user_mode(regs)) {
> -		if (!stack_access_ok(state, addr + sp_offset,
> -				     REGS_SIZE - SP_OFFSET))
> -			return false;
> +static bool deref_stack_iret_regs(struct unwind_state *state, unsigned long addr,
> +				  unsigned long *ip, unsigned long *sp)
> +{
> +	struct pt_regs *regs = (void *)addr - IRET_FRAME_OFFSET;

I guess those are traditionally done with container_of()...

But yeah, FWIW, looks ok to me:

Reviewed-by: Borislav Petkov <bp@...e.de>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.