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

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

On Mon, May 8, 2017 at 7:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> > And yes, I realize that there were other such bugs and that such bugs might
>> > occur in the future - but why not push the overhead of the security check to
>> > the kernel build phase? I.e. I'm wondering how well we could do static
>> > analysis during kernel build - would a limited mode of Sparse be good enough
>> > for that? Or we could add a new static checker to tools/, built from first
>> > principles and used primarily for extended syntactical checking.
>>
>> Static analysis is just not going to cover all cases. We've had vulnerabilities
>> where interrupt handlers left KERNEL_DS set, for example. [...]
>
> Got any commit ID of that bug - was it because a function executed by the
> interrupt handler leaked KERNEL_DS?

Ah, it was an exception handler, but the one I was thinking of was this:
https://lwn.net/Articles/419141/

>> [...] If there are performance concerns, let's put this behind a CONFIG. 2-5
>> instructions is not an issue for most people that want this coverage.
>
> That doesn't really _solve_ the performance concerns, it just forces most people
> to enable it by creating a 'security or performance' false dichotomy ...

That's fair, but what I'm trying to say is that many people will want
this, so rejecting it because it's 2 more instructions seems
unreasonable. We have had much more invasive changes added to the
kernel.

>> [...] and it still won't catch everything. Bug-finding is different from making
>> a bug class just unexploitable at all. As we've done before, it's the difference
>> between trying to find format string attacks vs just removing %n from the format
>> parser.
>
> No, it does not make it unexploitable, it could still be exploitable if the
> runtime check is buggy or if there's kernel execution outside of the regular
> system call paths - there's plenty of such hardware functionality on x86 for
> example.

Fine, but this is splitting hairs. This does protect a specific
situation, and it does so very cheaply. The real fix would be to
remove set_fs() entirely. :P

-Kees

-- 
Kees Cook
Pixel Security