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
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.