From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: ARC-Seal: i=1; a=rsa-sha256; t=1526626433; cv=none; d=google.com; s=arc-20160816; b=F1QW5vefZD3NYEZR9hT3ELn/Lqu1hIB3xKN/a55asOGSDc3HCSNW8eLtces4mnb99f TsPDQyBZ2WIFQWuZO+9zErEAfMvN9oLqzq5/YOlnT+8T42PUWJHwW9uymr8gV2ZRRPnY Ry5GBDDjMHvQELrFId7OtH9bv1IjdQJuECQeqhATcIpgVfioJ0Qopvg2Mk9RXqAM1pIr q6qSCIIBt3htfN2pgW6lC/+eqB1wwRX0qTGSfzQzWNUuP7KcZrHycz5+XSNGM0dO6rGH Mt2+UDymLGHrJXLuRF3T5wA62s01fJePaaFUTjE+QvT9pu+7pi4VA76HbJQagm9Cc4MJ 7Ikw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:sender:dkim-signature :arc-authentication-results; bh=D1+YeNlMta+IWwBJdi2XOvOjbp2xEpIZYd4wDYujxk0=; b=TGNFg0yY7yMkk54hLj+LHN96A6b2fA0FW/okmY78SwofmzX3t2Us29Om+sbEk6PHdw 2Us/1aMLvb2YPWF9Ua4Lwo6ynMsdCwyI+Hm/MGb5cjuIDmoQcqeJXk12WectXR9K7gma p7plR74lZxvaCapJn6Ir+c4Gu3BEZGDSrzmN9iKK/gij+6iG4IZZC8uMzk7enmUW6NjT 9ES7jDxxk+fwzX2ui20yf4NjCVjp6wbEOYEwwW9SERs3yb0I2Ccrb7Y6W0EQMVatk8ai jz5TXKOVEK+Hwm3Ypcu+rqS/RbqdB394JBUREuCNRUxsh7+MjZFx60Jl6QOC0i2bbA7M 8jLw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=GRAS4Bdi; spf=pass (google.com: domain of mingo.kernel.org@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=mingo.kernel.org@gmail.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=GRAS4Bdi; spf=pass (google.com: domain of mingo.kernel.org@gmail.com designates 209.85.220.65 as permitted sender) smtp.mailfrom=mingo.kernel.org@gmail.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org X-Google-Smtp-Source: AB8JxZon9VQSGqls0+1hC9hhENNVju9yZgqivtBvVEIM7fMmfOPSnjQPTo/Cpt/cIyTZUA7FLK6P+g== Sender: Ingo Molnar Date: Fri, 18 May 2018 08:53:49 +0200 From: Ingo Molnar To: Alexander Popov Cc: kernel-hardening@lists.openwall.com, Kees Cook , PaX Team , Brad Spengler , Andy Lutomirski , Tycho Andersen , Laura Abbott , Mark Rutland , Ard Biesheuvel , Borislav Petkov , Richard Sandiford , Thomas Gleixner , "H . Peter Anvin" , Peter Zijlstra , "Dmitry V . Levin" , Emese Revfy , Jonathan Corbet , Andrey Ryabinin , "Kirill A . Shutemov" , Thomas Garnier , Andrew Morton , Alexei Starovoitov , Josef Bacik , Masami Hiramatsu , Nicholas Piggin , Al Viro , "David S . Miller" , Ding Tianhong , David Woodhouse , Josh Poimboeuf , Steven Rostedt , Dominik Brodowski , Juergen Gross , Linus Torvalds , Greg Kroah-Hartman , Dan Williams , Dave Hansen , Mathias Krause , Vikas Shivappa , Kyle Huey , Dmitry Safonov , Will Deacon , Arnd Bergmann , Florian Weimer , Boris Lukashev , Andrey Konovalov , x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v12 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Message-ID: <20180518065349.GA10080@gmail.com> References: <1526488097-20611-1-git-send-email-alex.popov@linux.com> <1526488097-20611-3-git-send-email-alex.popov@linux.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1526488097-20611-3-git-send-email-alex.popov@linux.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1600638811678325053?= X-GMAIL-MSGID: =?utf-8?q?1600783839189179388?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: * Alexander Popov wrote: > --- a/arch/x86/entry/calling.h > +++ b/arch/x86/entry/calling.h > @@ -329,8 +329,22 @@ For 32-bit we have the following conventions - kernel is built with > > #endif > > +.macro ERASE_KSTACK_NOCLOBBER > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + PUSH_AND_CLEAR_REGS > + call erase_kstack > + POP_REGS > +#endif > +.endm > + > #endif /* CONFIG_X86_64 */ > > +.macro ERASE_KSTACK > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + call erase_kstack > +#endif > +.endm Please use a well-organized, common, visually easy to ignore namespace. For example: STACKLEAK_ERASE_NOCLOBBER > @@ -298,6 +300,7 @@ ENTRY(ret_from_fork) > /* When we fork, we trace the syscall return in the child, too. */ > movl %esp, %eax > call syscall_return_slowpath > + ERASE_KSTACK Ditto: STACKLEAK_ERASE etc. > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -32,6 +32,7 @@ struct vm86; > #include > #include > #include > +#include > mm_segment_t addr_limit; > > + struct lowest_stack lowest_stack; This too should be something more organized and more opaque, like: struct stackleak_info stackleak_info; And the field name should not be a meaningless 'val', but 'lowest_stack'. I.e. "p->stackleak_info.lowest_stack", which is so much more informative ... > --- a/arch/x86/kernel/process_32.c > +++ b/arch/x86/kernel/process_32.c > @@ -136,6 +136,11 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp, > p->thread.sp0 = (unsigned long) (childregs+1); > memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps)); > > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + p->thread.lowest_stack.val = (unsigned long)end_of_stack(p) + > + sizeof(unsigned long); > +#endif This should use an inline helper: stackleak_task_init(p); > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK > + p->thread.lowest_stack.val = (unsigned long)end_of_stack(p) + > + sizeof(unsigned long); > +#endif Beyond the lower visual impact this duplication will be removed by the inline helper as well. > +++ b/kernel/stackleak.c > @@ -0,0 +1,72 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This code fills the used part of the kernel stack with a poison value > + * before returning to the userspace. It's a part of the STACKLEAK feature > + * ported from grsecurity/PaX. > + * > + * Author: Alexander Popov > + * > + * STACKLEAK reduces the information which kernel stack leak bugs can > + * reveal and blocks some uninitialized stack variable attacks. Moreover, > + * STACKLEAK blocks stack depth overflow caused by alloca (aka Stack Clash > + * attack). > + */ s/alloca /alloca() > +#include > +#include > +#include > +#include Yeah, so since processor.h includes stackleak.h I strongly doubt the stackleak.h inclusion is necessary here. Please review every header inclusion line and remove the unnecessary ones. > + > +asmlinkage void erase_kstack(void) This too should be in the stackleak_*() namespace. > +{ > + /* > + * It would be nice not to have p and boundary on the stack. > + * Setting the register specifier for them is the best we can do. > + */ > + register unsigned long p = current->thread.lowest_stack.val; > + register unsigned long boundary = p & ~(THREAD_SIZE - 1); Does the 'register' keyword actually have any effect on the generated code? > + unsigned long poison = 0; > + const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH / > + sizeof(unsigned long); Please don't break lines in such an ugly fashion! Also, 'poison' is a very weird name for something that looks like an index. Plus since it's bound by "check_depth" is the 'unsigned long' justified, or could it be 32-bit? > + > + /* > + * Let's search for the poison value in the stack. > + * Start from the lowest_stack and go to the bottom. > + */ > + while (p > boundary && poison <= check_depth) { > + if (*(unsigned long *)p == STACKLEAK_POISON) > + poison++; > + else > + poison = 0; > + > + p -= sizeof(unsigned long); > + } This comment would be so much easier to read if the initialization was done right before the first use, i.e.: /* * Let's search for the poison value in the stack. * Start from the lowest_stack and go to the bottom: */ p = current->thread.lowest_stack.val; boundary = p & ~(THREAD_SIZE - 1); while (p > boundary && poison <= check_depth) { if (*(unsigned long *)p == STACKLEAK_POISON) poison++; else poison = 0; ... > + > + /* > + * One long int at the bottom of the thread stack is reserved and > + * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK). > + */ > + if (p == boundary) > + p += sizeof(unsigned long); Please put types into quotes where it's ambigous. I first read this sentence as "One long ..." and went "wtf". It's a totally unnecessary disruption of the reading flow. > + /* > + * So let's write the poison value to the kernel stack. > + * Start from the address in p and move up till the new boundary. > + * We assume that the stack pointer doesn't change when we write poison. > + */ Here too 'p' is easier to read. But 'p' is a very weird name: in the kernel it's usually some sort of process pointer. Please rename it to something more descriptive, such as "kstack_ptr" or so. > + if (on_thread_stack()) > + boundary = current_stack_pointer; > + else > + boundary = current_top_of_stack(); > + > + BUG_ON(boundary - p >= THREAD_SIZE); Please make this: if ( WARN_ON_ONCE()) return; ... or so, so that if this code is buggy we get actual useful user reports, not just "my machine froze, help!"... > + /* Reset the lowest_stack value for the next syscall */ > + current->thread.lowest_stack.val = current_top_of_stack() - 256; Magic, unexplained '256' literal. Thanks, Ingo