* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
@ 2012-10-10 6:46 Todd Poynor
2012-10-10 11:15 ` Dave Martin
0 siblings, 1 reply; 9+ messages in thread
From: Todd Poynor @ 2012-10-10 6:46 UTC (permalink / raw)
To: linux-arm-kernel
Invalid frame pointer (signed) -4 <= fp <= -1 defeats check for too high
on overflow.
Signed-off-by: Todd Poynor <toddpoynor@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 */
--
1.7.7.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-10-10 6:46 [PATCH] ARM: backtrace: avoid crash on large invalid fp value Todd Poynor
@ 2012-10-10 11:15 ` Dave Martin
2012-11-02 23:47 ` Colin Cross
0 siblings, 1 reply; 9+ messages in thread
From: Dave Martin @ 2012-10-10 11:15 UTC (permalink / raw)
To: linux-arm-kernel
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@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-10-10 11:15 ` Dave Martin
@ 2012-11-02 23:47 ` Colin Cross
2012-11-05 10:54 ` Dave Martin
0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2012-11-02 23:47 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 10, 2012 at 4:15 AM, Dave Martin <dave.martin@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@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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-11-02 23:47 ` Colin Cross
@ 2012-11-05 10:54 ` Dave Martin
2012-11-09 2:05 ` Colin Cross
0 siblings, 1 reply; 9+ messages in thread
From: Dave Martin @ 2012-11-05 10:54 UTC (permalink / raw)
To: linux-arm-kernel
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@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@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-11-05 10:54 ` Dave Martin
@ 2012-11-09 2:05 ` Colin Cross
2012-11-09 10:56 ` Dave Martin
0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2012-11-09 2:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Nov 5, 2012 at 2:54 AM, Dave Martin <dave.martin@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@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@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).
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-11-09 2:05 ` Colin Cross
@ 2012-11-09 10:56 ` Dave Martin
2012-11-09 18:17 ` Colin Cross
0 siblings, 1 reply; 9+ messages in thread
From: Dave Martin @ 2012-11-09 10:56 UTC (permalink / raw)
To: linux-arm-kernel
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@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@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@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-11-09 10:56 ` Dave Martin
@ 2012-11-09 18:17 ` Colin Cross
2012-11-13 9:49 ` Dave Martin
0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2012-11-09 18:17 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 9, 2012 at 2:56 AM, Dave Martin <dave.martin@linaro.org> wrote:
> 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@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@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@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.
current_thread_info() is assuming a sane stack, where the sp is
between [THREAD_SIZE * X + sizeof(struct thread_info), THREAD_SIZE *
(X + 1) - 8] (see THREAD_START_SP). It should never see sp =
THREAD_SIZE * X, so we shouldn't be copying its behavior in that case.
sp = THREAD_SIZE * x being a full stack would mean that the stack has
passed all the way through the struct thread_info stored at the lower
addresses of the stack, corrupting the task struct, saved registers,
and likely the stack too. On the other hand, sp = THREAD_SIZE * x
being an empty stack would mean somebody started a stack higher than
THREAD_START_SP. Neither one really makes sense, maybe I should just
validate the sp above the thread_info and below THREAD_START_SP.
> 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 agree with using - 1 (or - 4) to prevent high wrapping, but maybe
capping at THREAD_START_SP would simplify the code.
> 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.
I'll squash it in to mine.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
2012-11-09 18:17 ` Colin Cross
@ 2012-11-13 9:49 ` Dave Martin
0 siblings, 0 replies; 9+ messages in thread
From: Dave Martin @ 2012-11-13 9:49 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 09, 2012 at 10:17:01AM -0800, Colin Cross wrote:
> On Fri, Nov 9, 2012 at 2:56 AM, Dave Martin <dave.martin@linaro.org> wrote:
> > 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@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@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@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.
>
> current_thread_info() is assuming a sane stack, where the sp is
> between [THREAD_SIZE * X + sizeof(struct thread_info), THREAD_SIZE *
> (X + 1) - 8] (see THREAD_START_SP). It should never see sp =
> THREAD_SIZE * X, so we shouldn't be copying its behavior in that case.
>
> sp = THREAD_SIZE * x being a full stack would mean that the stack has
> passed all the way through the struct thread_info stored at the lower
> addresses of the stack, corrupting the task struct, saved registers,
> and likely the stack too. On the other hand, sp = THREAD_SIZE * x
> being an empty stack would mean somebody started a stack higher than
> THREAD_START_SP. Neither one really makes sense, maybe I should just
> validate the sp above the thread_info and below THREAD_START_SP.
>
> > 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 agree with using - 1 (or - 4) to prevent high wrapping, but maybe
> capping at THREAD_START_SP would simplify the code.
Having a more precise check as you describe seems to be a good thing.
I'm happy to go with your judgement.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ARM: backtrace: avoid crash on large invalid fp value
@ 2012-05-08 7:49 Todd Poynor
0 siblings, 0 replies; 9+ messages in thread
From: Todd Poynor @ 2012-05-08 7:49 UTC (permalink / raw)
To: linux-arm-kernel
Wraparound on invalid frame pointer (signed) -4 <= fp <= -1
defeats check for too high.
Signed-off-by: Todd Poynor <toddpoynor@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 */
--
1.7.7.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-13 9:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-10 6:46 [PATCH] ARM: backtrace: avoid crash on large invalid fp value Todd Poynor
2012-10-10 11:15 ` Dave Martin
2012-11-02 23:47 ` Colin Cross
2012-11-05 10:54 ` Dave Martin
2012-11-09 2:05 ` Colin Cross
2012-11-09 10:56 ` Dave Martin
2012-11-09 18:17 ` Colin Cross
2012-11-13 9:49 ` Dave Martin
-- strict thread matches above, loose matches on Subject: below --
2012-05-08 7:49 Todd Poynor
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.