[PATCH] ARM: backtrace: avoid crash on large invalid fp value

[Dave Martin]

On Thu, Nov 08, 2012 at 06:05:52PM -0800, Colin Cross wrote:
> On Mon, Nov 5, 2012 at 2:54 AM, Dave Martin <dave.martin at linaro.org> wrote:
> > On Fri, Nov 02, 2012 at 04:47:38PM -0700, Colin Cross wrote:
> >> On Wed, Oct 10, 2012 at 4:15 AM, Dave Martin <dave.martin at linaro.org> wrote:
> >> > On Tue, Oct 09, 2012 at 11:46:12PM -0700, Todd Poynor wrote:
> >> >> Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high
> >> >> on overflow.
> >> >>
> >> >> Signed-off-by: Todd Poynor <toddpoynor at google.com>
> >> >> ---
> >> >>  arch/arm/kernel/stacktrace.c |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> >> >> index 00f79e5..6315162 100644
> >> >> --- a/arch/arm/kernel/stacktrace.c
> >> >> +++ b/arch/arm/kernel/stacktrace.c
> >> >> @@ -31,7 +31,7 @@ int notrace unwind_frame(struct stackframe *frame)
> >> >>       high = ALIGN(low, THREAD_SIZE);
> >> >>
> >> >>       /* check current frame pointer is within bounds */
> >> >> -     if (fp < (low + 12) || fp + 4 >= high)
> >> >> +     if (fp < (low + 12) || fp >= high - 4)
> >> >>               return -EINVAL;
> >> >>
> >> >>       /* restore the registers from the stack frame */
> >> >
> >> > sp and fp can still be complete garbage in the case of a corrupted frame,
> >> > so low + 12 can still overflow and cause us to read beyond the stack base.
> >> >
> >> > A more robust patch might be as follows.  This also checks for misaligned
> >> > fp and sp values, since those indicate corruption and there can be no
> >> > sensible way to interpret the resulting frame in that case.
> >> >
> >> > Also, according to the definition of current_thread_info(),
> >> > IS_ALIGNED(sp, THREAD_SIZE) indicates a full stack extending from sp
> >> > to sp + THREAD_SIZE, and not an empty stack extending from sp -
> >> > THREAD_SIZE to sp.  We cannot backtrace this situation anyway, since
> >> > that would imply that the frame record extends beyond the stack...
> >> > but this patch tidies it up in the interest of clarity.
> >> >
> >> > Cheers
> >> > ---Dave
> >> >
> >> > (untested)
> >> >
> >> > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> >> > index 00f79e5..fec82be 100644
> >> > --- a/arch/arm/kernel/stacktrace.c
> >> > +++ b/arch/arm/kernel/stacktrace.c
> >> > @@ -28,10 +28,20 @@ int notrace unwind_frame(struct stackframe *frame)
> >> >
> >> >         /* only go to a higher address on the stack */
> >> >         low = frame->sp;
> >> > -       high = ALIGN(low, THREAD_SIZE);
> >> > +       if (!IS_ALIGNED(fp, 4))
> >> > +               return -EINVAL;
> >> > +
> >> > +       /*
> >> > +        * low + 1 here ensures that high > sp, consistent with the
> >> > +        * definition of current_thread_info().
> >> > +        * We subtract 1 to compute the highest allowable byte address.
> >> > +        * Otherwise, we might get high == 0 which would confuse our
> >> > +        * comparisons.
> >> > +        */
> >> > +       high = ALIGN(low + 1, THREAD_SIZE) - 1;
> 
> ARM eabi stacks are full-descending, meaning that if the sp is a
> multiple of THREAD_SIZE, the stack is empty.  current_thread_info
> takes a short-cut and assumes it can never be called on an empty
> stack, but better not to propagate that anywhere else.

The effect of the code is consistent with current_thread_info():

	low = THREAD_SIZE * X --> high = THREAD_SIZE * (X + 1) - 1
	low = THREAD_SIZE * (X + 1) - 1 --> high = THREAD_SIZE * (X + 1) - 1

i.e., low = THREAD_SIZE * X is treated as a full stack.


The comment relates to the case where the stack is right at the top
of the address space: if we define high as ALIGN(low + 1, THREAD_SIZE),
then high overflow to zero in this case, giving unexpected results
for comparisons "some_address >= high".

Definig high as the address of the last byte of the stack (instead of
the first byte after the stack) avoids this kind of problem, providing
that "some_address >= high" is rewritten as "some_address > high" in
our comparisons.

I don't know whether any stack will be at the top of the address space
in practice, but I prefer to avoid unnecessary assumptions where
possible.


Do you agree with the code as-is, or does something need to be changed/
clarified?

> >> >
> >> >         /* check current frame pointer is within bounds */
> >> > -       if (fp < (low + 12) || fp + 4 >= high)
> >> > +       if (fp < 12 || fp - 12 < low || fp > high)
> >> >                 return -EINVAL;
> >> >
> >> >         /* restore the registers from the stack frame */
> >> > @@ -39,6 +49,10 @@ int notrace unwind_frame(struct stackframe *frame)
> >> >         frame->sp = *(unsigned long *)(fp - 8);
> >> >         frame->pc = *(unsigned long *)(fp - 4);
> >> >
> >> > +       /* Do not claim the frame is valid if if is obviously corrupt: */
> >> > +       if (!IS_ALIGNED(frame->fp, 4))
> >> > +               return -EINVAL;
> >> > +
> >> >         return 0;
> >> >  }
> >> >  #endif
> >> >
> >> >
> >> > _______________________________________________
> >> > linux-arm-kernel mailing list
> >> > linux-arm-kernel at lists.infradead.org
> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >> Dave or Todd, mind reposting this, or should I squash it into my
> >> CONFIG_SMP stacktrace series?
> >
> > I'm happy for you to fold my patch into your series if you agree
> > with it.  Ideally, please fix my typo in the final comment ("if IT is
> > obviously corrupt").
> >
> > Do I assume correctly that you are already testing this stuff?
> 
> I've been testing it by repeatedly dumping the stack of a running
> thread (cat /dev/urandom > /dev/null) and making sure it doesn't
> panic, and by dumping all the threads in a idle system and making sure
> they all end at the normal user or kernel thread initial frames
> (do_exit, kernel_thread_exit, or ret_fast_syscall).

OK -- that's good to know.

I'm still assuming that you're rolling this into your series.  Let me
know if you want me to post a separate patch.

Cheers
---Dave