Re: [RFC][PATCH] objtool: STAC/CLAC validation

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Andy Lutomirski <luto@kernel.org>
Cc: Denys Vlasenko <dvlasenk@redhat.com>,
	Brian Gerst <brgerst@gmail.com>,
	Julien Thierry <julien.thierry@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Will Deacon <will.deacon@arm.com>,
	valentin.schneider@arm.com, Ingo Molnar <mingo@redhat.com>,
	James Morse <james.morse@arm.com>,
	"H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	"linux-alpha@vger.kernel.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC][PATCH] objtool: STAC/CLAC validation
Date: Fri, 22 Feb 2019 17:12:33 -0800	[thread overview]
Message-ID: <CAHk-=wjmp0UugFnD2KaXyGO8RprfgULwMhseDhAxQQMqqL1Eiw@mail.gmail.com> (raw)
In-Reply-To: <CALCETrVw18nHQGPqg_OF=pD7NePafYQ_c1MhfSm_OCQ-O3xzhQ@mail.gmail.com>

On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> [mailing lists removed because this is a potentially large source of exploits]

I think you're overly worried.

AC doesn't protect against "large source of exploits". If it did, then
all CPU's before Broadwell would be insecure. They aren't. They'd
better not be, considering that there's a _lot_ of Xeon machines out
there based on older microarchitectures. I think some of them might
even be reasonably current (eg Xeon E7v3 isn't _that_ old, and is
Haswell-based, and doesn't have SMAP afaik).

SMAP is a great debugging and development aid, and makes sure that
developers - who hopefully run primarily on modern platforms - don't
write code that just accesses user space directly (because with SMAP,
it won't work).

And yes, SMAP can limit the effect of kernel bugs, and turn something
that would otherwise be a security issue into "just a bug".

But running with AC on isn't a security issue in itself - it just
makes SMAP slightly less powerful. The biggest issue of the whole "AC
doesn't get saved/redstored" is actually the *reverse* case, where a
preemption event could then cause a process that had AC on to be
scheduled away, then AC would stay on for some time, but then we might
schedule back with AC _clear_, and now you'd have a non-working user
access, and a possible DoS attack as a result because you returned
EFAULT to a system call that was perfectly fine.

See? It's not so much "AC stays on" that is a "sky is falling" issue,
it's actually "AC also gets turned off randomly" that actually has
real and immediate effects. "AC on" is unfortunate and not great,
don't get me wrong, but it's definitely not the end of the world.
Particularly not for short sequences.

That said:

> Um, wait a moment.  You didn't find an oddity in ptrace.c.  You found
> a giant freaking error in uaccess.h.

I agree that your patch is good, and should be applied. Mind writing
up a changelog and committing it to -tip?

> Am I missing something?  How are there not zillions of instances of
> this that your patch ought to catch?  Or is genregs_get() really the
> only example?

I really do think that it's very unusual to do "get/put_user()" with
complicated value arguments. So while your patch is obviously the
right thing to do, I really don't think this is a huge worry, or all
_that_ surprising that this issue apparently found just a single case
of a function call.

With all that said, I didn't even react to this part of PeterZ's
patch, but it's a good call, and I think it's also a great validation
of the objtool approach to validating AC. So cheers for that!

                Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel