[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170816211636.4zwublkvpn6hbo5n@smitten>
Date: Wed, 16 Aug 2017 15:16:36 -0600
From: Tycho Andersen <tycho@...ker.com>
To: Alexander Popov <alex.popov@...ux.com>
Cc: kernel-hardening@...ts.openwall.com, keescook@...omium.org,
pageexec@...email.hu, spender@...ecurity.net,
Laura Abbott <labbott@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Ard Biesheuvel <ard.biesheuvel@...aro.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 Alexander,
On Wed, Aug 16, 2017 at 11:47:44PM +0300, Alexander Popov wrote:
> Hello Tycho,
>
> On 15.08.2017 06:38, Tycho Andersen wrote:
> > On Wed, Jul 12, 2017 at 09:17:51PM +0300, Alexander Popov wrote:
> >> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> >> +void __used track_stack(void)
> >> +{
> >> + unsigned long sp = (unsigned long)&sp;
> >> +
> >> + if (sp < current->thread.lowest_stack &&
> >> + sp >= (unsigned long)task_stack_page(current) +
> >> + 2 * sizeof(unsigned long)) {
> >> + current->thread.lowest_stack = sp;
> >> + }
> >> +
> >> + if (unlikely((sp & ~(THREAD_SIZE - 1)) < (THREAD_SIZE / 16)))
> >
> > I think this check is wrong, the lhs should be
> > (sp & (THREAD_SIZE - 1)). Otherwise, we just check that the upper bits
> > of the stack are < THREAD_SIZE / 16, which they never will be.
>
> Thank you, I think you are right!
>
> I can additionally notice that this erroneous check is not a part of PaX patch,
> it is introduced by Grsecurity patch.
>
> Thanks again, I'll fix and annotate it in the next version of the patch.
>
> Did you manage to create a test for the correct check which hits the BUG()?
Yes, see below. I've fixed all of the review feedback from the last
time I posted it too. Feel free to add it to your tree and post it w/ the
next version if that makes the most sense.
Cheers,
Tycho
>From 2c8a8f96a331b63e3aa52388fab9f111c516bf1c 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.
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 | 135 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 142 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..daae36e0432e
--- /dev/null
+++ b/drivers/misc/lkdtm_stackleak.c
@@ -0,0 +1,135 @@
+/*
+ * 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>
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+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;
+ if ((unsigned long *) current->thread.lowest_stack < lowest)
+ lowest = (unsigned long *) current->thread.lowest_stack;
+
+ 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 (i > 32)
+ pr_warn_once("More than 256 bytes not canaried?");
+
+ 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;
+ }
+}
+#else
+bool check_my_stack(void)
+{
+ return false;
+}
+#endif
+
+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_warn("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;
+
+ /* use almost all of the stack, minus the buffer space allowed in
+ * track_stack and the space used by track_stack itself
+ */
+ do_alloca(left - THREAD_SIZE / 16 - sizeof(unsigned long), use_some_stack);
+}
--
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.
