From patchwork Thu Aug 17 17:58:07 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tycho Andersen X-Patchwork-Id: 9906837 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 7881760386 for ; Thu, 17 Aug 2017 17:59:18 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6DD4828AA8 for ; Thu, 17 Aug 2017 17:59:18 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 62A9C28B7C; Thu, 17 Aug 2017 17:59:18 +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.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID 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 E186F28AA8 for ; Thu, 17 Aug 2017 17:59:16 +0000 (UTC) Received: (qmail 28408 invoked by uid 550); 17 Aug 2017 17:58:22 -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 28384 invoked from network); 17 Aug 2017 17:58:21 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=docker.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=vJEc8BsanjHNLzlFe4L+0rreJpndrAKH2EItumv/UNQ=; b=dsDvPKpGoV2dL+c5+xnQZMCWPLHw2ieleIg3M3qrzM+wbubR6pMPGRL27SOkxAN1mj u+++udx9Imy1V+CjlmdnXitmlo6m8++x1UjeyUEjd59232aHvXKxcubj+PjUi1J4/Duc CFEfA5zW3nfhzgMRy3I7TTwPKQFAypCdak2xk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=vJEc8BsanjHNLzlFe4L+0rreJpndrAKH2EItumv/UNQ=; b=RhFNcqr0aO3AUB/xc9Rc0jyXW9aTZsjxyDazSLqnmHJ0vMYCe7Z+wLciB0OQ9d2Lt0 FwEiS+Fhj5gT+opTqtM0nZkB3z07zqbN678gak8Za8U8oobIE1pyM3pOKmpCm9Dy1g5i jRAVUfO+VlH58odr1I41fOVMFMn2OFr+lcj1c99yJl+bsvFJugdL+/k5NthKoKwPqMsL PCB2RSNvMP2JIMOfCYHtaekb/T8it0eHn9muJc+wY4SoohxXlyyuyKIUvfMaYa77uo1/ duCGRN5ENZ4TzbS5nnqB2s9sydtgeHdpa8/kdy/Ddtwfs7lpVtPI6Jeqr8IqDw7Iqgux N73Q== X-Gm-Message-State: AHYfb5iw28990YdK20VkAO08C3q8wSNogW6Oe88Bj23jsgfEByRSLdsA N97RxZjc64mqmTzD X-Received: by 10.36.103.69 with SMTP id u66mr2668266itc.46.1502992689590; Thu, 17 Aug 2017 10:58:09 -0700 (PDT) Date: Thu, 17 Aug 2017 11:58:07 -0600 From: Tycho Andersen To: Kees Cook Cc: Alexander Popov , "kernel-hardening@lists.openwall.com" , PaX Team , Brad Spengler , Laura Abbott , Mark Rutland , Ard Biesheuvel , "x86@kernel.org" , Andy Lutomirski Message-ID: <20170817175807.ejgsrfdgojgbtkwf@smitten> References: <1499883471-23822-1-git-send-email-alex.popov@linux.com> <20170815033834.2qbjj2of62udqyz3@smitten> <93382ef8-105f-eed4-0d97-0b1a66a047e6@linux.com> <20170816211636.4zwublkvpn6hbo5n@smitten> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: [kernel-hardening] Re: [PATCH RFC v3 1/1] gcc-plugins: Add stackleak feature erasing the kernel stack at the end of syscalls X-Virus-Scanned: ClamAV using ClamSMTP Hi Kees, Thanks for the review. On Wed, Aug 16, 2017 at 03:16:13PM -0700, Kees Cook wrote: > On Wed, Aug 16, 2017 at 2:16 PM, Tycho Andersen wrote: > > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > > I'd like tests to fail if the given config options are missing. That > way we're always running the same test code, but it's only the kernel > that is changing. If it's just STACKLEAK_POISON missing, we can just > #ifndef that and insert it manually here. Sounds good. It was also for the lowest_stack member of struct thread_struct, but that wasn't strictly necessary for the test to work correctly, it just means... > > + left -= 2 * sizeof(unsigned long); > > + > > + /* let's count the number of canaries, not bytes */ > > + left /= sizeof(unsigned long); > > + > > + for (i = 0; i < left; i++) { > > + if (*(lowest - i) != STACKLEAK_POISON) > > + continue; > > + > > + if (i > 32) > > + pr_warn_once("More than 256 bytes not canaried?"); > > Why the _once part here? ...that we should drop this warning, which was mostly a sanity check during development anyway. I believe I've addressed the rest as well with the version below. Cheers, Tycho From 5cae1f904cd3d813628a5b22ca5fe054b5eb7378 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Thu, 8 Jun 2017 12:43:07 -0600 Subject: [PATCH] lkdtm: add a test for STACKLEAK plugin There are two tests here, one to test that the BUG() in check_alloca is hit correctly, and the other to test that the BUG() in track_stack is hit correctly. Ideally we'd also be able to check end-to-end that a syscall results in an entirely poisoned stack, but I'm not sure how to do a syscall from lkdtm. v2: * use good comment style * drop references to lowest_stack, and #define STACKLEAK_POISON if necessary * drop unnecessary warning about canary space * add error messages, make them explicit, and use pr_err() Signed-off-by: Tycho Andersen --- drivers/misc/Makefile | 1 + drivers/misc/lkdtm.h | 4 ++ drivers/misc/lkdtm_core.c | 2 + drivers/misc/lkdtm_stackleak.c | 133 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+) diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 81ef3e67acc9..805e4f06011a 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -61,6 +61,7 @@ lkdtm-$(CONFIG_LKDTM) += lkdtm_heap.o lkdtm-$(CONFIG_LKDTM) += lkdtm_perms.o lkdtm-$(CONFIG_LKDTM) += lkdtm_rodata_objcopy.o lkdtm-$(CONFIG_LKDTM) += lkdtm_usercopy.o +lkdtm-$(CONFIG_LKDTM) += lkdtm_stackleak.o KCOV_INSTRUMENT_lkdtm_rodata.o := n diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h index 3b4976396ec4..3b67cc4a070b 100644 --- a/drivers/misc/lkdtm.h +++ b/drivers/misc/lkdtm.h @@ -64,4 +64,8 @@ void lkdtm_USERCOPY_STACK_FRAME_FROM(void); void lkdtm_USERCOPY_STACK_BEYOND(void); void lkdtm_USERCOPY_KERNEL(void); +/* lkdtm_stackleak.c */ +void lkdtm_STACKLEAK_ALLOCA(void); +void lkdtm_STACKLEAK_BIG_FRAME(void); + #endif diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c index 42d2b8e31e6b..f42b346bdf5c 100644 --- a/drivers/misc/lkdtm_core.c +++ b/drivers/misc/lkdtm_core.c @@ -235,6 +235,8 @@ struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_STACK_FRAME_FROM), CRASHTYPE(USERCOPY_STACK_BEYOND), CRASHTYPE(USERCOPY_KERNEL), + CRASHTYPE(STACKLEAK_ALLOCA), + CRASHTYPE(STACKLEAK_BIG_FRAME), }; diff --git a/drivers/misc/lkdtm_stackleak.c b/drivers/misc/lkdtm_stackleak.c new file mode 100644 index 000000000000..2fa44c641d11 --- /dev/null +++ b/drivers/misc/lkdtm_stackleak.c @@ -0,0 +1,133 @@ +/* + * This file tests a few aspects of the stackleak compiler plugin: + * - the current task stack somewhere below lowest_stack is properly canaried + * - small allocas are allowed properly via check_alloca() + * - big allocations that exhaust the stack are BUG()s + * - function calls whose stack frames blow the stack are BUG()s + * + * Copyright (C) Docker, Inc. 2017 + * + * Author: Tycho Andersen + */ + +#include "lkdtm.h" + +#include +#include + +/* for security_inode_init_security */ +#include + +#ifndef STACKLEAK_POISON +# define STACKLEAK_POISON -0xBEEF +#endif + +static bool check_poison(unsigned long *ptr, unsigned long n) +{ + unsigned long i; + + for (i = 1; i < n; i++) { + if (*(ptr - i) != STACKLEAK_POISON) + return false; + } + + return true; +} + +static bool check_my_stack(void) +{ + unsigned long *lowest, left, i; + + lowest = &i; + left = (unsigned long) lowest % THREAD_SIZE; + + /* + * See note in arch/x86/entry/entry_64.S about the or; the bottom two + * qwords are not + */ + left -= 2 * sizeof(unsigned long); + + /* let's count the number of canaries, not bytes */ + left /= sizeof(unsigned long); + + for (i = 0; i < left; i++) { + if (*(lowest - i) != STACKLEAK_POISON) + continue; + + if (!check_poison(lowest - i, 16)) + continue; + + break; + } + + if (i == left) { + pr_warn("didn't find canary?"); + return false; + } + + if (check_poison((unsigned long *) lowest - i, left - i)) { + pr_info("current stack poisoned correctly\n"); + return true; + } else { + pr_err("current stack not poisoned correctly\n"); + return false; + } +} + +static noinline void do_alloca(unsigned long size, void (*todo)(void)) +{ + char buf[size]; + + if (todo) + todo(); + + /* so this doesn't get inlined or optimized out */ + snprintf(buf, size, "hello world\n"); +} + +/* Check the BUG() in check_alloca() */ +void lkdtm_STACKLEAK_ALLOCA(void) +{ + unsigned long left = (unsigned long) &left % THREAD_SIZE; + + if (!check_my_stack()) + return; + + /* try a small allocation to see if it works */ + do_alloca(16, NULL); + pr_info("small allocation successful\n"); + + + pr_info("attempting large alloca of %lu\n", left); + do_alloca(left, NULL); + pr_err("FAIL: large alloca succeded!\n"); +} + +static void use_some_stack(void) { + + /* + * Note: this needs to be a(n exported) function that has track_stack + * inserted, i.e. it isn't in the various sections restricted by + * stackleak_track_stack_gate. + */ + security_inode_init_security(NULL, NULL, NULL, NULL, NULL); +} + +/* + * Note that the way this test fails is kind of ugly; it hits the BUG() in + * track_stack, but then the BUG() handler blows the stack and hits the stack + * guard page. + */ +void lkdtm_STACKLEAK_BIG_FRAME(void) +{ + unsigned long left = (unsigned long) &left % THREAD_SIZE; + + if (!check_my_stack()) + return; + + /* + * use almost all of the stack up to the padding allowed by track_stack + */ + do_alloca(left - THREAD_SIZE / 16 - 1, use_some_stack); + pr_err("FAIL: stack frame should have blown stack!\n"); +}