[PATCH] ARM: backtrace: avoid crash on large invalid fp value
ccross at google.com
Fri Nov 2 19:47:38 EDT 2012
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.
> 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;
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
Dave or Todd, mind reposting this, or should I squash it into my
CONFIG_SMP stacktrace series?
More information about the linux-arm-kernel