* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).