From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753989AbdK0RgS (ORCPT ); Mon, 27 Nov 2017 12:36:18 -0500 Received: from mail.skyhub.de ([5.9.137.197]:43270 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753917AbdK0RgQ (ORCPT ); Mon, 27 Nov 2017 12:36:16 -0500 Date: Mon, 27 Nov 2017 18:36:04 +0100 From: Borislav Petkov To: Ingo Molnar Cc: linux-kernel@vger.kernel.org, Dave Hansen , Andy Lutomirski , Thomas Gleixner , "H . Peter Anvin" , Peter Zijlstra , Linus Torvalds Subject: Re: [PATCH 02/21] x86/unwinder: Handle stack overflows more gracefully Message-ID: <20171127173604.eff4ta576mpxjhjg@pd.tnic> References: <20171127104529.12435-1-mingo@kernel.org> <20171127104529.12435-3-mingo@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20171127104529.12435-3-mingo@kernel.org> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 27, 2017 at 11:45:10AM +0100, Ingo Molnar wrote: > From: Josh Poimboeuf > > 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 > Cc: Andy Lutomirski > Cc: Borislav Petkov > Cc: Brian Gerst > Cc: Dave Hansen > Cc: Linus Torvalds > Cc: Peter Zijlstra > Cc: Thomas Gleixner > Link: http://lkml.kernel.org/r/20171126024031.uxi4numpbjm5rlbr@treble > Signed-off-by: Ingo Molnar > --- > 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 > #include > > +#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 ®s->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 -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.