linux-hardening - Re: [PATCH] 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] [day] [month] [year] [list]

Message-ID: <202408121113.D638922E4@keescook>
Date: Mon, 12 Aug 2024 11:22:00 -0700
From: Kees Cook <kees@...nel.org>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: 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] slab: Introduce kmalloc_obj() and family

On Fri, Aug 09, 2024 at 10:59:52AM +0200, Vlastimil Babka wrote:
> On 8/8/24 01:54, 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 = kcalloc(count, 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(p, gfp);
> > 	kzalloc_obj(p, count, gfp);
> > 	kmalloc_obj(p, count, gfp);
> > 	kzalloc_obj(p, count, gfp);
> > 	kmalloc_obj(p, flex_member, count, gfp);
> 
> So I'm not a huge fan in hiding the assignment, but I understand there's
> value in having guaranteed the target of the assignment is really the same
> thing as the one used for sizeof() etc.
> 
> But returning size seems awkward, it would be IMHO less confusing if it
> still returned the object pointer, that could be then also assigned
> elsewhere if needed, tested for NULL and ZERO_SIZE_PTR (now it's both 0?).

Ah, I made this changed based on requests in earlier threads. But yes,
the ambiguity with ZERO_SIZE_PTR does make me uncomfortable too. I think
I can make the size an optional argument when splitting the functions as
you request below...

> I'm also not sure that having it all called kmalloc_obj() with 3 variants of
> how many parameters it takes is such a win? e.g. kmalloc_obj(),
> kcalloc_obj() and kcalloc_obj_flex() would be more obvious?

I liked it because it's always doing the same thing: allocating a
structure. But yes, I do see that it's a bit weird. Since "kcalloc" means
"zero it also", how about the naming just uses pluralization instead?

	kmalloc_obj(p, gfp);
	kmalloc_objs(p, count, gfp);
	kmalloc_flex(p, flex_member, count, gfp);

Does that looks okay?

> > These each return the size of the allocation, so that other common
> > idioms can be converted easily as well. For example:
> > 
> > 	info->size = struct_size(ptr, flex_member, count);
> > 	ptr = kmalloc(info->size, gfp);
> > 
> > becomes:
> > 
> > 	info->size = kmalloc_obj(ptr, flex_member, count, gfp);
> 
> How about instead taking an &info->size parameter that assigns size to it,
> so the ptr can be still returned but we also can record the size?

If we wanted size output, we could add an optional final argument:

	kmalloc_obj(p, gfp, &size);
	kmalloc_objs(p, count, gfp, &size);
	kmalloc_flex(p, flex_member, count, gfp, &size);

And as far as solving the concern of "hiding the assignment", what about
trying to "show" that "p" is being assigned by requiring that it also
use "&"? For example:

	kmalloc_obj(&p, gfp);
	kmalloc_objs(&p, count, gfp);
	kmalloc_flex(&p, flex_member, count, gfp);

> Also the last time David asked for documentation, you say you would try, but
> there's nothing new here? Dunno if the kerneldocs are feasible but there's
> at least Documentation/core-api/memory-allocation.rst ...

Whoops, yes. I totally missed adding those. I will add that once I have
some direction on the above design ideas.

Thanks for looking at this!

-Kees

-- 
Kees Cook