linux-kernel - Re: [PATCH v4 0/3] Hardening perf subsystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202406121530.D9DB956C8@keescook>
Date: Wed, 12 Jun 2024 16:23:31 -0700
From: Kees Cook <kees@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Erick Archer <erick.archer@...look.com>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Matthew Wilcox <mawilcox@...rosoft.com>, x86@...nel.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v4 0/3] Hardening perf subsystem

On Thu, Jun 13, 2024 at 12:08:21AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 12, 2024 at 12:01:19PM -0700, Kees Cook wrote:
> > I'm happy to take patches. And for this bikeshed, this would be better
> > named under the size_*() helpers which are trying to keep size_t
> > calculations from overflowing (by saturating). i.e.:
> > 
> > 	size_add_mult(sizeof(*p), sizeof(*p->member), num)
> 
> Fine I suppose, but what if we want something not size_t? Are we waiting
> for the type system extension?

Because of C's implicit promotion/truncation, we can't do anything
sanely with return values of arbitrary type size; we have to capture the
lvalue type somehow so the checking can happen without C doing silent
garbage.

> The saturating thing is relying in the allocators never granting INT_MAX
> (or whatever size_t actually is) bytes?

The max of size_t is ULONG_MAX, but yes, most of the allocators will
refuse >INT_MAX, but I think vmalloc() is higher, but certainly not
SIZE_MAX, which is the entire virtual memory space. ;)

The saturating thing is two-fold: that we never wrap around SIZE_MAX,
and that the allocator will refuse a SIZE_MAX allocation.

> > LOL. It's basically doing compile-time (__builtin_object_size) and
> > run-time (__builtin_dynamic_object_size) bounds checking on destination
> > (and source) object sizes, mainly driven by the mentioned builtins:
> > https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html
> 
> Right, I got that far. I also read most of:
> 
>   https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854

Oh wow, that's serious extra credit. :) It'll also probably be a while
before most of that stuff is even landed in Clang, much less implemented
in GCC. What we _do_ have is the "counted_by" attribute. This was added
to Clang a little while ago and just landed last week in GCC for GCC 15.

> But none of that is showing me generated asm for the various cases. As
> such, I don't consider myself informed enough.

Gotcha. For the compile-time stuff it's all just looking at
known-at-compile-time sizes. So for something like this, we get a
__compiletime_warning() emitted:

	const char src[] = "Hello there";
	char dst[10];

	strscpy(dst, src); /* Compiler yells since src is bigger than dst. */

For run-time checks it's basically just using the regular WARN()
infrastructure with __builtin_dynamic_object_size(). Here's a simplified
userspace example with assert():

https://godbolt.org/z/zMrKnMxn5

The kernel's FORTIFY_SOURCE is much more complex in how it does the
checking, how it does the reporting (for helping people figure out what's
gone weird), etc.

> > Anyway! What about the patch that takes the 2 allocations down to 1?
> > That seems like an obvious improvement.
> 
> Separate it from the struct_size() nonsense and Cc the author of that
> code (Sandipan IIRC) and I might just apply it.

Okay, thanks!

-Kees

-- 
Kees Cook