Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up

[Josh Poimboeuf]

On Sun, Aug 14, 2016 at 12:56:40AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > Add a sanity check to ensure the stack only grows down, and print a
> > warning if the check fails.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > ---
> >  arch/x86/kernel/unwind_frame.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> > index 5496462..f21b7ef 100644
> > --- a/arch/x86/kernel/unwind_frame.c
> > +++ b/arch/x86/kernel/unwind_frame.c
> > @@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
> >  }
> >  EXPORT_SYMBOL_GPL(unwind_get_return_address);
> >
> > +static size_t regs_size(struct pt_regs *regs)
> > +{
> > +       /* x86_32 regs from kernel mode are two words shorter */
> > +       if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
> > +               return sizeof(*regs) - (2*sizeof(long));
> > +
> > +       return sizeof(*regs);
> > +}
> > +
> >  static bool is_last_task_frame(struct unwind_state *state)
> >  {
> >         unsigned long bp = (unsigned long)state->bp;
> > @@ -85,6 +94,7 @@ bool unwind_next_frame(struct unwind_state *state)
> >         struct pt_regs *regs;
> >         unsigned long *next_bp, *next_sp;
> >         size_t next_len;
> > +       enum stack_type prev_type = state->stack_info.type;
> >
> >         if (unwind_done(state))
> >                 return false;
> > @@ -140,6 +150,18 @@ bool unwind_next_frame(struct unwind_state *state)
> >         if (!update_stack_state(state, next_sp, next_len))
> >                 goto bad_address;
> >
> > +       /* make sure it only unwinds up and doesn't overlap the last frame */
> > +       if (state->stack_info.type == prev_type) {
> > +               if (state->regs &&
> > +                   (void *)next_sp < (void *)state->regs +
> > +                                     regs_size(state->regs))
> > +                       goto bad_address;
> > +
> > +               if (state->bp &&
> > +                   (void *)next_sp < (void *)state->bp + FRAME_HEADER_SIZE)
> > +                       goto bad_address;
> > +       }
> > +
> 
> Maybe this is obvious in context, but does something prevent this
> error from firing if the stack switched?  That is:
> 
> pushq $rbp
> movq $rsp, $rbp
> ...
> movq [irq stack], $rsp
> <- rsp and rbp have no particular relationship right now.

Short answer:

No, because the above warning only happens between two "frame" pointers
(where frame pointer might be a regs pointer) when the two frames are on
the same stack.  This warning has nothing to do with the stack pointer,
despite the "next_sp" name.  I should probably rename "next_sp" to
"next_frame" or something.

Long answer:

The unwinder is frame-pointer based, so in most cases it completely
ignores the value of the stack pointer.  The only exceptions are:

a) in __unwind_start() where it can use the value of regs->sp to determine how
   many frames to skip; and

b) when reading the next stack pointer to switch to the next stack.

If for example the regs where taken from an interrupt right after the
stack had been switched, then no frame would contain the stack pointer,
and __unwind_start() would skip all the frames, and the unwind would be
reported as empty.

Such edge cases are exceedingly rare, and are acceptable IMO, because
frame pointers and interrupts are inherently not 100% compatible.  And
these edge cases already exist in today's code.

(And I should reiterate that even when the unwinder breaks down like
that, the oops dump code should still keep going and show all the
addresses anyway.)

-- 
Josh