Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 21 Sep 2017 22:39:02 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: Tycho Andersen <tycho@...ker.com>
Cc: Kees Cook <keescook@...omium.org>,
 "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

On 21.09.2017 00:18, Tycho Andersen wrote:
> On Wed, Sep 20, 2017 at 02:27:05PM +0300, Alexander Popov wrote:
>> On 17.08.2017 20:58, Tycho Andersen wrote:
>>> 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.
>>
>> Could you please elaborate on this? I didn't get the idea.
> 
> Sorry, I realized I never replied to this comment. The idea would be
> to simulate an entire syscall as a user would do, from beginning to
> end, so that we can ensure all the machinery works as it is supposed
> to (i.e. when the syscall returns, we can check that the task's stack
> is completely poisoned).

A quick search gives that Greg's article:
https://www.linuxjournal.com/article/8110
He shows the examples of syscalls from the kernelspace. But it might not be
enough, since the actual stack erasing is happening during kernel -> user
context switch. Do you think the idea is possible at all?

> Below is an updated patch based on your feedback. Thanks!

Please see the comments below.

> From 34f68b92ac2f2a8d770765e47ae3612d5bb29fae 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.
> 
> v3: * fix whitespace in casts
>     * consistently use FAIL for errors
>     * drop extra whitespace
>     * fix up unaligned stack logic
>     * print the number of unpoisoned bytes on successful check
> 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>

[...]

> diff --git a/drivers/misc/lkdtm_stackleak.c b/drivers/misc/lkdtm_stackleak.c
> new file mode 100644
> index 000000000000..8849500e7bc9
> --- /dev/null
> +++ b/drivers/misc/lkdtm_stackleak.c
> @@ -0,0 +1,142 @@
> +/*
> + * This file tests a few aspects of the stackleak compiler plugin:
> + *   - the current task stack is properly canaried

Here and further: maybe "poisoned" or "erased" is better than "canaried" since
STACKLEAK_POISON is not used as a canary.

> + *   - 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 noinline bool check_poison(unsigned long *ptr, unsigned long n)
> +{
> +	unsigned long i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (*(ptr - i) != STACKLEAK_POISON)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool check_my_stack(void)
> +{
> +	unsigned long *lowest, canaries, 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 canaried.
> +	 */

Do you mean "are not poisoned" here? STACK_END_MAGIC at the bottom of the stack
is indeed used as canary.

I would also suggest to make this comment less x86_64-specific, since your test
will be used on other platforms too.

> +	left -= 2 * sizeof(unsigned long);
> +
> +	/*
> +	 * Check each byte, as we don't know the current stack alignment.
> +	 */

Excuse me, what do you mean by the "current stack alignment"?

The STACKLEAK_POISON position is always 8-byte aligned for x86_64 and 4-byte
aligned for x86 (see the shr instruction in the asm implementation).

I would suggest to simply align lowest. In case of unaligned poison, the test
will not find it and report the FAIL (which is correct).

> +	for (i = 0; i < left; i++) {
> +		if (*(lowest - i) != STACKLEAK_POISON)
> +			continue;
> +
> +		if (!check_poison((void *)lowest - i, 16))
> +			continue;
> +
> +		break;
> +	}

You've dropped "left /= sizeof(unsigned long)" and now the logic in this loop is
broken - it gives the kernel crash with disabled STACKLEAK.

IMHO counting like in your v2 is better, you just need to fix the off-by-one error.

> +
> +	if (i == left) {
> +		pr_err("FAIL: didn't find canary?\n");
> +		return false;
> +	}
> +
> +	if (i % sizeof(unsigned long)) {
> +		pr_err("FAIL: unaligned canary?\n");
> +		return false;
> +	}
> +
> +	/*
> +	 * How many canaries (not bytes) do we actually need to check?
> +	 */

Ditto about canaries vs poison values.

> +	canaries = (left - i) / sizeof(unsigned long *);
> +
> +	if (check_poison((void *)lowest - i, canaries)) {
> +		pr_info("stack poisoned correctly, %lu unpoisoned bytes\n", i);
> +		return true;
> +	} else {
> +		pr_err("FAIL: 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.
> + */

If we agree on making CONFIG_GCC_PLUGIN_STACKLEAK depend on CONFIG_VMAP_STACK
(and dropping the BUG() in track_stack()), the STACKLEAK_BIG_FRAME test case can
be omitted and there will be only one lkdtm_STACKLEAK test.

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

Thanks.
Best regards,
Alexander

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.