From patchwork Tue Feb 7 17:03:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Morse X-Patchwork-Id: 9560695 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 87250602B1 for ; Tue, 7 Feb 2017 17:04:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 75D99269A3 for ; Tue, 7 Feb 2017 17:04:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 66AC32842A; Tue, 7 Feb 2017 17:04:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id 3D3EE269A3 for ; Tue, 7 Feb 2017 17:04:54 +0000 (UTC) Received: (qmail 30702 invoked by uid 550); 7 Feb 2017 17:04:52 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 30672 invoked from network); 7 Feb 2017 17:04:51 -0000 Message-ID: <5899FDDD.3080605@arm.com> Date: Tue, 07 Feb 2017 17:03:25 +0000 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: kpark3469@gmail.com CC: kernel-hardening@lists.openwall.com, catalin.marinas@arm.com, keescook@chromium.org, will.deacon@arm.com, mark.rutland@arm.com, panand@redhat.com, keun-o.park@darkmatter.ae References: <1486296850-16045-1-git-send-email-kpark3469@gmail.com> <1486296850-16045-2-git-send-email-kpark3469@gmail.com> <58999F15.3090807@arm.com> In-Reply-To: <58999F15.3090807@arm.com> Subject: [kernel-hardening] Re: [PATCH v3 2/3] arm64: usercopy: Implement stack frame object validation X-Virus-Scanned: ClamAV using ClamSMTP Hi Sahara, On 07/02/17 10:19, James Morse wrote: > On 05/02/17 12:14, kpark3469@gmail.com wrote: >> From: Sahara >> >> This implements arch_within_stack_frames() for arm64 that should >> validate if a given object is contained by a kernel stack frame. >> >> Signed-off-by: Sahara > I'd like to avoid having two sets of code that walk the stack. > I will have a go at a version of this patch that uses arm64s existing > walk_stackframe() machinery - lets find out if there is a good reason not to do > it that way! The reason turns out to be because LKDTM isn't testing whether we are overlapping stack frames. Instead it wants us to tell it whether the original caller somewhere down the stack pointed into a stack frame that hadn't yet been written. This requires this function to know how it will be called and unwind some number of frames. Annoyingly we have to maintain start/end boundaries for each frame in case the object was neatly contained in a frame that wasn't written at the time. Ideally we would inline this stack check into the caller, testing object_end doesn't lie in the range current_stack_pointer : end_of_stack. This would work even when built without CONFIG_FRAME_POINTER but is probably too-invasive a change. As is, the walk_stackframe() machinery version looks like this: ---------------------------------------%<--------------------------------------- frame.graph = tsk->curr_ret_stack; @@ -215,3 +219,73 @@ void save_stack_trace(struct stack_trace *trace) } EXPORT_SYMBOL_GPL(save_stack_trace); #endif + +struct check_frame_arg { + unsigned long obj_start; + unsigned long obj_end; + unsigned long frame_start; + int discard_frames; + int err; +}; + +static int check_frame(struct stackframe *frame, void *d) +{ + struct check_frame_arg *arg = d; + unsigned long frame_end = frame->fp; + + /* object overlaps multiple frames */ + if (arg->obj_start < frame->fp && frame->fp < arg->obj_end) { + arg->err = BAD_STACK; + return 1; + } + + /* + * Discard frames and check object is in a frame written early + * enough. + */ + if (arg->discard_frames) + arg->discard_frames--; + else if ((arg->frame_start <= arg->obj_start && arg->obj_start < frame_end) && + (arg->frame_start < arg->obj_end && arg->obj_end <= frame_end)) + return 1; + + /* object exists in a previous frame */ + if (arg->obj_end < arg->frame_start) { + arg->err = BAD_STACK; + return 1; + } + + arg->frame_start = frame_end + 0x10; + + return 0; +} + +/* Check obj doesn't overlap a stack frame record */ +enum stack_type arch_within_stack_frames(const void *stack, + const void *stack_end, + const void *obj, unsigned long obj_len) +{ + struct stackframe frame; + struct check_frame_arg arg; + + if (!IS_ENABLED(CONFIG_FRAME_POINTER)) + return NOT_STACK; + + arg.err = GOOD_FRAME; + arg.obj_start = (unsigned long)obj; + arg.obj_end = arg.obj_start + obj_len; + + FAKE_FRAME(frame, arch_within_stack_frames); + arg.frame_start = frame.fp; + + /* + * Skip 4 non-inlined frames: , + * arch_within_stack_frames(), check_stack_object() and + * __check_object_size(). + */ + arg.discard_frames = 4; + + walk_stackframe(current, &frame, check_frame, &arg); + + return arg.err; +} ---------------------------------------%<--------------------------------------- This avoids the __builtin_frame_address(!0) problem and doesn't duplicate the actual stack walking. This can be simplified further if we can get rid of the time-travel requirements. It is unfortunately more code, but it is hopefully clearer! Thanks, James diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 8a552a33c6ef..620fa74201b5 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -25,6 +25,12 @@ #include #include +#define FAKE_FRAME(frame, my_func) do { \ + frame.fp = (unsigned long)__builtin_frame_address(0); \ + frame.sp = current_stack_pointer; \ + frame.pc = (unsigned long)my_func; \ +} while (0) + /* * AArch64 PCS assigns the frame pointer to x29. * @@ -194,9 +200,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) frame.pc = thread_saved_pc(tsk); } else { data.no_sched_functions = 0; - frame.fp = (unsigned long)__builtin_frame_address(0); - frame.sp = current_stack_pointer; - frame.pc = (unsigned long)save_stack_trace_tsk; + FAKE_FRAME(frame, save_stack_trace_tsk); } #ifdef CONFIG_FUNCTION_GRAPH_TRACER