linux-hardening - Re: [PATCH v3] slab: Introduce kmalloc

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <202408271709.31D322019@keescook>
Date: Tue, 27 Aug 2024 17:19:39 -0700
From: Kees Cook <kees@...nel.org>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Roman Gushchin <roman.gushchin@...ux.dev>,
	Hyeonggon Yoo <42.hyeyoo@...il.com>,
	"Gustavo A . R . Silva" <gustavoars@...nel.org>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, Jann Horn <jannh@...gle.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Marco Elver <elver@...gle.com>, linux-mm@...ck.org,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3] slab: Introduce kmalloc_obj() and family

On Tue, Aug 27, 2024 at 11:32:14PM +0200, Vlastimil Babka wrote:
> +Cc Linus
> 
> On 8/23/24 01:13, Kees Cook wrote:
> > Introduce type-aware kmalloc-family helpers to replace the common
> > idioms for single, array, and flexible object allocations:
> > 
> > 	ptr = kmalloc(sizeof(*ptr), gfp);
> > 	ptr = kzalloc(sizeof(*ptr), gfp);
> > 	ptr = kmalloc_array(count, sizeof(*ptr), gfp);
> > 	ptr = kcalloc(count, sizeof(*ptr), gfp);
> > 	ptr = kmalloc(struct_size(ptr, flex_member, count), gfp);
> > 
> > These become, respectively:
> > 
> > 	kmalloc_obj(ptr, gfp);
> > 	kzalloc_obj(ptr, gfp);
> > 	kmalloc_objs(ptr, count, gfp);
> > 	kzalloc_objs(ptr, count, gfp);
> > 	kmalloc_flex(ptr, flex_member, count, gfp);
> 
> This is indeed better than the previous version. The hidden assignment to
> ptr seems still very counter-intuitive, but if it's the only way to do those
> validations, the question is then just whether it's worth the getting used
> to it, or not.

We could make the syntax require "&ptr"?

As for alternatives, one thing I investigated for a while that made
several compiler people unhappy was to introduce an builtin named
something like __builtin_lvalue() which could be used on the RHS of an
assignment to discover the lvalue in some way. Then we could, for
example, add alignment discovery like so:

#define kmalloc(_size, _gfp)	\
	__kmalloc(_size, __alignof(typeof(__builtin_lvalue())), _gfp)

or do the FAM struct allocations:

#define kmalloc_flex(_member, _count, _gfp)	\
	__kmalloc(sizeof(*typeof(__builtin_lvalue())) +
		  sizeof(*__builtin_lvalue()->_member) * (_count), gfp)

Compiler folks seems very unhappy with this, though. As I can recognize
it creates problems for stuff like:

	return kmalloc(...)

Of course the proposed macros still have the above problem, and both to
use a temporary variable to deal with it.

So, really it's a question of "how best to introspect the lvalue?"

> [...]
> > by GCC[1] and Clang[2]. The internal use of __flex_count() allows for
> > automatically setting the counter member of a struct's flexible array
> 
> But if it's a to-be-implemented feature, perhaps it would be too early to
> include it here? Were you able to even test that part right now?

There are RFC patches for both GCC and Clang that I tested against.
However, yes, it is still pretty early. I just wanted to show that it
can work, etc. (i.e. not propose a macro with no "real" benefit over the
existing assignments).

> [...]
> > Replacing all existing simple code patterns found via Coccinelle[3]
> > shows what could be replaced immediately (saving roughly 1,500 lines):
> > 
> >  7040 files changed, 14128 insertions(+), 15557 deletions(-)
> 
> Since that could be feasible to apply only if Linus ran that directly
> himself, including him now. Because doing it any other way would leave us
> semi-converted forever and not bring the full benefits?

Right -- I'd want to do a mass conversion and follow it up with any
remaining ones. There are a lot in the style of "return k*alloc(...)"
for example.

> [...]
> > +#define kvmalloc_obj(P, FLAGS)				\
> > +	__alloc_objs(kvmalloc, P, 1, FLAGS, NULL)
> 
> Wonder if there is really a single struct (not array) with no flex array
> that could need kvmalloc? :)

Ah, yes, Good point. I was going for "full" macro coverage. :P

Thanks for looking at this!

-Kees

-- 
Kees Cook