diff mbox

Re: [PATCH RFC v3 1/1] gcc-plugins: Add stackleak feature erasing the kernel stack at the end of syscalls

Message ID 20170817175807.ejgsrfdgojgbtkwf@smitten (mailing list archive)
State New, archived
Headers show

Commit Message

Tycho Andersen Aug. 17, 2017, 5:58 p.m. UTC 
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@docker.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@docker.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@docker.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(+)

Comments

Alexander Popov Sept. 20, 2017, 11:27 a.m. UTC | #1 
Hello Tycho and Kees,

Please see the comments below.

On 17.08.2017 20:58, Tycho Andersen wrote:
> From 5cae1f904cd3d813628a5b22ca5fe054b5eb7378 Mon Sep 17 00:00:00 2001
> From: Tycho Andersen <tycho@docker.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.

Could you please elaborate on this? I didn't get the idea.

> 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@docker.com>

[...]

> 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

You've dropped the reference to the lowest_stack in v2.

> + *   - 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@docker.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;

Here and in other places type cast should not have a whitespace after it, right?

> +
> +	/*
> +	 * 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;
> +	}

What do you think about printing the number of bytes between *lowest and the
poisoned part of the stack?

> +
> +	if (i == left) {
> +		pr_warn("didn't find canary?");

Maybe pr_err("FAIL:...") here for the consistency?

> +		return false;
> +	}
> +
> +	if (check_poison((unsigned long *) lowest - i, left - i)) {
> +		pr_info("current stack poisoned correctly\n");

1. You cast here, but didn't do that above.

2. IMO, there is an error here. You don't check one last poison value at the
bottom of the stack. Your loop in check_poison() starts from 1, so it actually
checks (left - i - 1) values.

3. And I would suggest "stack is poisoned correctly".

> +		return true;
> +	} else {
> +		pr_err("current stack not poisoned correctly\n");

I would suggest pr_err("FAIL: stack is 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");
> +
> +

Two blank lines here.

> +	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.
> + */

Yes, actually, the reason is deeper.

When there are less than (THREAD_SIZE / 16) bytes left in the kernel stack, the
BUG() in track_stack() is hit. But do_error_trap(), which handles the invalid
opcode, has a big stack frame. So it is instrumented by the STACKLEAK gcc plugin
and itself calls track_stack() at the beginning. Hence we have a recursive
BUG(), which eventually hits the guard page.

I banned the instrumentation of do_error_trap() in the plugin, but it didn't
really help, since there are several other instrumented functions called during
BUG() handling.

So it seems to me that this BUG() in track_stack() is really useless and can be
dropped. Moreover:
 - it is not a part of the PaX patch;
 - it never worked in Grsecurity kernel because of the error spotted by Tycho.

What do you think about it?

> +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");
> +}
> 

Best regards,
Alexander
diff mbox

Patch

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@docker.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");
+}