From patchwork Wed Jan 30 01:55:45 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Colin Cross X-Patchwork-Id: 2064921 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 E0B063FDD1 for ; Wed, 30 Jan 2013 01:59:22 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1U0MuZ-0005qW-LA; Wed, 30 Jan 2013 01:56:27 +0000 Received: from mail-ye0-f202.google.com ([209.85.213.202]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1U0MuD-0005oT-MJ for linux-arm-kernel@lists.infradead.org; Wed, 30 Jan 2013 01:56:08 +0000 Received: by mail-ye0-f202.google.com with SMTP id r9so110385yen.5 for ; Tue, 29 Jan 2013 17:56:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references:x-gm-message-state; bh=mSUl1qdwrITOgGrZGjpa/p9mERUMGX6L51B8OL1pqLQ=; b=Dut1mS3gw/7j00z8496HxREfdRafXLmzhxs6enE+rkW4yF6y7kqoVro1Qu9o0WPVZ7 /Nuov2FjcTlMnfAQm8ROS2Qi8I7byz6bzLz9myf5dStXmL9dsIyzF4RXAiUtQex0TemQ +Au6feucZC4TYCtykYHHpvhnvRXWzGlCYDbnEcOpAaYliGDyBDOVDEuIIywRYgqpUyV/ sgMWTsF3Kf2XXXAtp7E5/aswzgC2bl7nG1Ynx1ik5hl35/+8VbwTdlXycQZizScaibsZ aJz3ZVPu1duCy/T4gA9HZjuUnpV5eDOp25Ft5SX6dEHaHZEa4zfgAchKjp8uD8jvv7Ei Smdg== X-Received: by 10.236.200.65 with SMTP id y41mr1342804yhn.36.1359510964141; Tue, 29 Jan 2013 17:56:04 -0800 (PST) Received: from corp2gmr1-2.hot.corp.google.com (corp2gmr1-2.hot.corp.google.com [172.24.189.93]) by gmr-mx.google.com with ESMTPS id k58si603787yhi.4.2013.01.29.17.56.04 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Tue, 29 Jan 2013 17:56:04 -0800 (PST) Received: from walnut.mtv.corp.google.com (walnut.mtv.corp.google.com [172.18.105.48]) by corp2gmr1-2.hot.corp.google.com (Postfix) with ESMTP id E6D1D5A42AF; Tue, 29 Jan 2013 17:56:03 -0800 (PST) Received: by walnut.mtv.corp.google.com (Postfix, from userid 99897) id 927161607A8; Tue, 29 Jan 2013 17:56:03 -0800 (PST) From: Colin Cross To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/3] ARM: stacktrace: harden FP stacktraces against invalid stacks Date: Tue, 29 Jan 2013 17:55:45 -0800 Message-Id: <1359510947-3950-2-git-send-email-ccross@android.com> X-Mailer: git-send-email 1.8.1 In-Reply-To: <1359510947-3950-1-git-send-email-ccross@android.com> References: <1359510947-3950-1-git-send-email-ccross@android.com> X-Gm-Message-State: ALoCoQnCtT65KwS6S17ua4CKDGWaGuDZt73regHpmWCLtX73i2m+ndMNqjFEvQC0Q6SVju5z5UzrJ/qEIMEI30x1Mff7VCGAH3XoOuWvwfPGQ8yb1u4PWgruM00sq4zEk+P11j+QICWyfXuyr6UR9lGCpvx8d/3lEMZh3p3LAawtGw4nyW6238fBA2PDXjDZniFmRCxRUoN7ys9xoq9TPx2WGN2RMeetew== X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130129_205605_914116_66C254E5 X-CRM114-Status: GOOD ( 26.66 ) X-Spam-Score: -3.3 (---) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-3.3 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.213.202 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Dave Martin , Laura Abbott , Catalin Marinas , Will Deacon , Rabin Vincent , Colin Cross , Russell King 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: , MIME-Version: 1.0 Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org Dumping stacktraces is currently disabled in ARM SMP for all tasks except the current task due to the worry that the task may be running on another CPU and that the unwinder may be unstable when presented with a stack that is being modified. Unwinding with CONFIG_FRAME_POINTER is fairly simple compared to when CONFIG_ARM_UNWIND is set. The next frame's FP and SP registers are read from the stack and can be validated against the current values to ensure that they do not leave the stack and make progress towards the upper end of the stack. This guarantees that accesses do not fault and that execution is bounded. Add additional validations to the version of unwind_frame used when CONFIG_FRAME_POINTER=y: Verify that the stack is in a mapped region of kernel memory. Fixes crashes seen in unwind_frame on real systems, although stack corruption caused by memory instability is likely the cause of the invalid sp and fp values. Fix address comparison to catch fp >= 0xfffffffc correctly. Fixes crash reported by Todd Poynor. Ensure the stack pointer moves to a higher address between each frame to make sure repeated calls to unwind_frame can't loop forever. Includes ideas from Dave Martin. Signed-off-by: Colin Cross --- v2: verify that initial sp value is in mapped lowmem verify that stack offsets are in the range [sizeof(struct thread_info), THREAD_START_SP) export stack validation functions for use by unwind v3: avoid using page structs to convert vaddr to pfn arch/arm/include/asm/stacktrace.h | 4 ++ arch/arm/kernel/stacktrace.c | 105 +++++++++++++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h index 4d0a164..6909056 100644 --- a/arch/arm/include/asm/stacktrace.h +++ b/arch/arm/include/asm/stacktrace.h @@ -12,4 +12,8 @@ struct stackframe { extern void walk_stackframe(struct stackframe *frame, int (*fn)(struct stackframe *, void *), void *data); +bool sp_addr_valid(unsigned long sp); +bool addr_in_stack(unsigned long orig_sp, unsigned long vsp); +bool sp_in_stack(unsigned long orig_sp, unsigned long vsp); + #endif /* __ASM_STACKTRACE_H */ diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c index 00f79e5..aba76d5 100644 --- a/arch/arm/kernel/stacktrace.c +++ b/arch/arm/kernel/stacktrace.c @@ -1,9 +1,89 @@ #include +#include #include #include #include +#define STACK_MAX(sp) (round_down(sp, THREAD_SIZE) + THREAD_START_SP) + +/** + * sp_addr_valid - verify a stack pointer + * @sp: current stack pointer + * + * Returns true if sp is a pointer inside a memory area that could be a stack. + * Does not verify that sp is inside an actual stack (i.e. does not check for + * STACK_MAGIC). + * + * If sp_addr_valid(sp) returns true, then the kernel will not fault if it + * accesses memory in the range + * [sp, round_down(sp, THREAD_SIZE) + THREAD_START_SP) + */ +bool sp_addr_valid(unsigned long sp) +{ + unsigned long high; + unsigned long offset; + unsigned int pfn; + unsigned int start_pfn; + unsigned int end_pfn; + + if (!IS_ALIGNED(sp, 4)) + return false; + + offset = sp & (THREAD_SIZE - 1); + + if (offset > THREAD_START_SP) + return false; + + if (offset < sizeof(struct thread_info)) + return false; + + high = STACK_MAX(sp); + + if (!virt_addr_valid(sp) || !virt_addr_valid(high)) + return false; + + start_pfn = __phys_to_pfn(__virt_to_phys(sp)); + end_pfn = __phys_to_pfn(__virt_to_phys(high)); + for (pfn = start_pfn; pfn <= end_pfn; pfn++) + if (!pfn_valid(pfn)) + return false; + + return true; +} + +/** + * addr_in_stack - verify a pointer is inside a specified stack + * @orig_sp: stack pointer at the bottom of the stack + * @sp: address to be verified + * + * Returns true if sp is in the stack bounded at the bottom by orig_sp, in the + * range [orig_sp, round_down(orig_sp, THREAD_SIZE) + THREAD_START_SP) + * + * If orig_sp is valid (see sp_addr_valid), then the kernel will not fault if it + * accesses a pointer where ptr_in_stack returns true. + */ +bool addr_in_stack(unsigned long orig_sp, unsigned long sp) +{ + return (sp >= orig_sp && sp < STACK_MAX(orig_sp) && IS_ALIGNED(sp, 4)); +} + +/** + * sp_in_stack - verify a stack pointer is inside a specified stack + * @orig_sp: stack pointer at the bottom of the stack + * @sp: stack pointer to be verified + * + * Returns true if sp is in the stack bounded at the bottom by orig_sp, in the + * range [orig_sp, round_down(orig_sp, THREAD_SIZE) + THREAD_START_SP] + * + * If sp_in_stack returns true, + * addr_in_stack(vsp, x) == addr_in_stack(orig_sp, x) + */ +bool sp_in_stack(unsigned long orig_sp, unsigned long sp) +{ + return (sp >= orig_sp && sp <= STACK_MAX(orig_sp) && IS_ALIGNED(sp, 4)); +} + #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) /* * Unwind the current stack frame and store the new register values in the @@ -23,15 +103,17 @@ */ int notrace unwind_frame(struct stackframe *frame) { - unsigned long high, low; unsigned long fp = frame->fp; + unsigned long sp = frame->sp; + + if (!sp_addr_valid(sp)) + return -EINVAL; - /* only go to a higher address on the stack */ - low = frame->sp; - high = ALIGN(low, THREAD_SIZE); + /* Check current frame pointer is within the stack bounds. */ + if (!addr_in_stack(sp, fp)) + return -EINVAL; - /* check current frame pointer is within bounds */ - if (fp < (low + 12) || fp + 4 >= high) + if (fp < 12 || !addr_in_stack(sp, fp - 12)) return -EINVAL; /* restore the registers from the stack frame */ @@ -39,6 +121,17 @@ int notrace unwind_frame(struct stackframe *frame) frame->sp = *(unsigned long *)(fp - 8); frame->pc = *(unsigned long *)(fp - 4); + /* Ensure the next stack pointer is in the same stack */ + if (!sp_in_stack(sp, frame->sp)) + return -EINVAL; + + /* + * Ensure the next stack pointer is above this frame to guarantee + * bounded execution. + */ + if (frame->sp < fp) + return -EINVAL; + return 0; } #endif