From mboxrd@z Thu Jan 1 00:00:00 1970 From: dave.martin@linaro.org (Dave Martin) Date: Fri, 9 Nov 2012 10:56:48 +0000 Subject: [PATCH] ARM: backtrace: avoid crash on large invalid fp value In-Reply-To: References: <1349851572-9967-1-git-send-email-toddpoynor@google.com> <20121010111517.GC2131@linaro.org> <20121105105421.GB2005@linaro.org> Message-ID: <20121109105648.GA2048@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Nov 08, 2012 at 06:05:52PM -0800, Colin Cross wrote: > On Mon, Nov 5, 2012 at 2:54 AM, Dave Martin 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 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 > >> >> --- > >> >> 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