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

[Colin Cross]

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.

>> >
>> >         /* 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).