Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

From: Kees Cook <keescook@chromium.org>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: "Peter Zijlstra" <peterz@infradead.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Brian Gerst" <brgerst@gmail.com>,
	"Kernel Hardening" <kernel-hardening@lists.openwall.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Heiko Carstens" <heiko.carstens@de.ibm.com>,
	"Oleg Nesterov" <oleg@redhat.com>,
	"David Howells" <dhowells@redhat.com>,
	"Dave Hansen" <dave.hansen@intel.com>,
	"H . Peter Anvin" <hpa@zytor.com>,
	"Ingo Molnar" <mingo@kernel.org>,
	"Pavel Tikhomirov" <ptikhomirov@virtuozzo.com>,
	linux-s390 <linux-s390@vger.kernel.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"René Nyffenegger" <mail@renenyffenegger.ch>,
	"Greg KH" <greg@kroah.com>,
	"Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
	"Rik van Riel" <riel@redhat.com>, "Arnd Bergmann" <arnd@arndb.de>,
	"Will Deacon" <will.deacon@arm.com>,
	"Borislav Petkov" <bp@alien8.de>,
	"Al Viro" <viro@zeniv.linux.org.uk>,
	"Andy Lutomirski" <luto@kernel.org>,
	"Josh Poimboeuf" <jpoimboe@redhat.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"Linux API" <linux-api@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Daniel Micay" <danielmicay@gmail.com>,
	"James Morse" <james.morse@arm.com>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	"Martin Schwidefsky" <schwidefsky@de.ibm.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Thomas Garnier" <thgarnie@google.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode
Date: Fri, 12 May 2017 14:04:49 -0700	[thread overview]
Message-ID: <CAGXu5jJEoTEkOdbVu+AQF8=_NRem3g5vECJYU=n8UNpfHbBHTA@mail.gmail.com> (raw)
In-Reply-To: <CAGXu5jL6FPuShBpZfi6+XHqOk4gxocUJRYPHT5oR3HYh3xm+sA@mail.gmail.com>

On Fri, May 12, 2017 at 2:00 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, May 12, 2017 at 1:45 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Fri, May 12, 2017 at 10:30:44PM +0200, Peter Zijlstra wrote:
>>> On Fri, May 12, 2017 at 09:21:06PM +0100, Russell King - ARM Linux wrote:
>>> > On Fri, May 12, 2017 at 12:30:02PM -0700, Kees Cook wrote:
>>> > > I'm clearly not explaining things well enough. I shouldn't say
>>> > > "corruption", I should say "malicious manipulation". The methodology
>>> > > of attacks against the stack are quite different from the other kinds
>>> > > of attacks like use-after-free, heap overflow, etc. Being able to
>>> > > exhaust the kernel stack (either due to deep recursion or unbounded
>>> > > alloca())
>>> >
>>> > I really hope we don't have alloca() use in the kernel.  Do you have
>>> > evidence to support that assertion?
>>> >
>>> > IMHO alloca() (or similar) should not be present in any kernel code
>>> > because we have a limited stack - we have kmalloc() etc for that kind
>>> > of thing.
>>>
>>> On stack variable length arrays get implemented by the compiler doing
>>> alloca(), and we sadly have a few of those around.
>>
>> I hope their size is appropriately limited, but something tells me it
>> would be foolish to assume that.
>>
>>> But yes, fully agreed on the desirability of alloca() and things.
>>
>> Hmm, I wonder if -fno-builtin-alloca would prevent those... it looks
>> like it certainly would prevent an explicit alloca() call.
>
> Building with -Werror=vla is exciting. :)
>
> A lot of it is in crypto (which are relatively static sizes, just
> using function callbacks), but there is plenty more.

I meant to also paste an example (which is harmless, I haven't looked
extensively at other examples):

        unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
        unsigned int size = crypto_tfm_alg_blocksize(tfm);
        u8 buffer[size + alignmask];

Looking at all the places (and having tried to remove a few of these
in pstore), I think it might be quite frustrating to eliminate them
all and then declare VLAs dead. I'm not against trying, though. :)

-Kees

-- 
Kees Cook
Pixel Security