Re: [PATCH] slab: Decouple slab_debug and no_hash_pointers

[Kees Cook]

On Mon, Apr 14, 2025 at 02:31:42PM +0200, Petr Mladek wrote:
> On Thu 2025-04-10 10:44:31, Kees Cook wrote:
> > Some system owners use slab_debug=FPZ (or similar) as a hardening option,
> > but do not want to be forced into having kernel addresses exposed due
> > to the implicit "no_hash_pointers" boot param setting.[1]
> > 
> > Introduce the "hash_pointers" boot param, which defaults to "auto"
> > (the current behavior), but also includes "always" (forcing on hashing
> > even when "slab_debug=..." is defined), and "never". The existing
> > "no_hash_pointers" boot param becomes an alias for "hash_pointers=never".
> > 
> > This makes it possible to boot with "slab_debug=FPZ hash_pointers=always".
> 
> The idea makes sense. But it seems that the patch did not handle
> the "always" mode correctly, see below.

Actually, it was the "never" mode that was being ignored. Whoops! (The
double negation language is a little odd.) I've fixed this for v2 with
an explicit switch statement.

> > -int __init no_hash_pointers_enable(char *str)
> > +void __init hash_pointers_finalize(bool slub_debug)
> >  {
> > -	if (no_hash_pointers)
> > -		return 0;
> > +	if (hash_pointers_mode == HASH_PTR_AUTO && slub_debug)
> > +		no_hash_pointers = true;
> >  
> > -	no_hash_pointers = true;
> > +	if (!no_hash_pointers)
> > +		return;
> >  
> >  	pr_warn("**********************************************************\n");
> >  	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> 
> 
> The mode/policy is generic but this function is ready to be called
> only once. And we might actually want to call it twice, see below.

I'd like to keep it a single call. I feel this simplifies the reporting
logic, keeps the selection logic in one place, and allows us to
trivially examine that it is safe to use with __init.

Thanks for the review!

-- 
Kees Cook