From patchwork Wed Oct 10 11:15:17 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: tip-bot for Dave Martin X-Patchwork-Id: 1572961 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id 87E6F40135 for ; Wed, 10 Oct 2012 11:17:21 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TLuG7-0003T6-EZ; Wed, 10 Oct 2012 11:15:27 +0000 Received: from mail-bk0-f49.google.com ([209.85.214.49]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TLuG3-0003Sf-Gv for linux-arm-kernel@lists.infradead.org; Wed, 10 Oct 2012 11:15:24 +0000 Received: by mail-bk0-f49.google.com with SMTP id j4so214833bkw.36 for ; Wed, 10 Oct 2012 04:15:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :x-gm-message-state; bh=v7roMNn1olyfcGa6cUwDEgFG26FWipo5etnTZe+Ki3M=; b=ewHonn90m1/EemZ3ScEeulH7dF1PWA1TNDknAB1Q4U5bE6es7igAMU1Stz/IraYrIP fERj3Fln40lRgTrJ959lin1BJlEVNGhQdGsrAQqzZRncjUIiWLZjDQiy52lAa7cxSFDZ AaILPNuHgWYFgFh8TLxSpSYRahgbujVLPZoMKo2AKdEBhO5Am/syUKU/M99v6todfeq1 XmyxinKvInRsPJtLgko61q1TMtym/kpjK8Hj3qJVc9tWn2bNe9z9qBY8fPXLzW05iAN2 Ldz6wQFadYn8rfHVqO0SoZPCrhpK39u8ZY3tBkK0aEkWz9m1Nmwt6rkaw40FDQKl4U99 7rxw== Received: by 10.204.11.141 with SMTP id t13mr4923715bkt.65.1349867721026; Wed, 10 Oct 2012 04:15:21 -0700 (PDT) Received: from linaro.org (fw-lnat.cambridge.arm.com. [217.140.96.63]) by mx.google.com with ESMTPS id v14sm881829bkv.10.2012.10.10.04.15.19 (version=SSLv3 cipher=OTHER); Wed, 10 Oct 2012 04:15:19 -0700 (PDT) Date: Wed, 10 Oct 2012 12:15:17 +0100 From: Dave Martin To: Todd Poynor Subject: Re: [PATCH] ARM: backtrace: avoid crash on large invalid fp value Message-ID: <20121010111517.GC2131@linaro.org> References: <1349851572-9967-1-git-send-email-toddpoynor@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1349851572-9967-1-git-send-email-toddpoynor@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQnhFYnw4xdIdYrkFqBjxThMNu76q7xBuYdH57uIPeNA71xSiSwq7xAyEGgax9GbR9/4Aw7j X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.214.49 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Russell King , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org 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; /* 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