Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 17 Aug 2017 11:58:07 -0600
From: Tycho Andersen <tycho@...ker.com>
To: Kees Cook <keescook@...omium.org>
Cc: Alexander Popov <alex.popov@...ux.com>,
	"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
	PaX Team <pageexec@...email.hu>,
	Brad Spengler <spender@...ecurity.net>,
	Laura Abbott <labbott@...hat.com>,
	Mark Rutland <mark.rutland@....com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	"x86@...nel.org" <x86@...nel.org>,
	Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH RFC v3 1/1] gcc-plugins: Add stackleak feature erasing
 the kernel stack at the end of syscalls

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 <tycho@...ker.com> 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 <tycho@...ker.com>
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 <tycho@...ker.com>
---
 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 <tycho@...ker.com>
+ */
+
+#include "lkdtm.h"
+
+#include <linux/sched.h>
+#include <linux/compiler.h>
+
+/* for security_inode_init_security */
+#include <linux/security.h>
+
+#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");
+}
-- 
2.11.0

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.