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

[Dave Martin]

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;
> >
> >         /* 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?

Cheers
---Dave