Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 20 Sep 2017 08:13:09 -0600
From: Tycho Andersen <tycho@...ker.com>
To: Alexander Popov <alex.popov@...ux.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 Wed, Sep 20, 2017 at 02:27:05PM +0300, Alexander Popov wrote:
> > +/*
> > + * 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?

We'll only have a stack guard page in the case of vmap stack, so maybe
we can do:

diff --git a/fs/exec.c b/fs/exec.c
index 8333c4dce59b..8351369cd1e4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1960,7 +1960,8 @@ void __used track_stack(void)
 		current->thread.lowest_stack = sp;
 	}
 
-	if (unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16)))
+	if (!IS_ENABLED(CONFIG_VMAP_STACK) &&
+	    unlikely((sp & (THREAD_SIZE - 1)) < (THREAD_SIZE / 16)))
 		BUG();
 }
 EXPORT_SYMBOL(track_stack);

Anyway, thanks for the reviews, I'll post an updated version shortly.

Tycho

Powered by blists - more mailing lists

Your e-mail address:

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