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
