| Message ID | 20250410174428.work.488-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | slab: Decouple slab_debug and no_hash_pointers | expand |
On 4/10/25 19:44, 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". > > Link: https://github.com/KSPP/linux/issues/368 [1] > Fixes: 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled") > Co-developed-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Kees Cook <kees@kernel.org> I like how this makes things more generic. Perhaps there are more debug boot/config options that could tie into the hash_pointers=auto and are even more obvious than slab_debug in the sense that you would really only enable them in debugging/CI runs when you do not care about the info leaks but want as much useful debug info as possible (KASAN etc?). Given how this changes mostly printk code and is in fact only a small change to slab, I'll wait first if printk maintainers want to take this patch. In that case please add Acked-by: Vlastimil Babka <vbabka@suse.cz> Thanks!
On Thu, 10 Apr 2025, 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". > > Link: https://github.com/KSPP/linux/issues/368 [1] > Fixes: 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled") > Co-developed-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Kees Cook <kees@kernel.org> Acked-by: David Rientjes <rientjes@google.com>
On Thu, Apr 10, 2025 at 10:44:31AM -0700, 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". > > Link: https://github.com/KSPP/linux/issues/368 [1] > Fixes: 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled") > Co-developed-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com> > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Christoph Lameter <cl@linux.com> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Roman Gushchin <roman.gushchin@linux.dev> > Cc: Harry Yoo <harry.yoo@oracle.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Tamir Duberstein <tamird@gmail.com> > Cc: Miguel Ojeda <ojeda@kernel.org> > Cc: Alice Ryhl <aliceryhl@google.com> > Cc: linux-doc@vger.kernel.org > Cc: linux-mm@kvack.org > --- > .../admin-guide/kernel-parameters.txt | 34 ++++++++----- > include/linux/sprintf.h | 2 +- > lib/vsprintf.c | 51 +++++++++++++++++-- > mm/slub.c | 5 +- > 4 files changed, 72 insertions(+), 20 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 76e538c77e31..d0fd9c745db9 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1798,6 +1798,27 @@ > backtraces on all cpus. > Format: 0 | 1 > > + hash_pointers= > + [KNL,EARLY] > + By default, when pointers are printed to the console > + or buffers via the %p format string, that pointer is > + "hashed", i.e. obscured by hashing the pointer value. > + This is a security feature that hides actual kernel > + addresses from unprivileged users, but it also makes > + debugging the kernel more difficult since unequal > + pointers can no longer be compared. The choices are: > + Format: { auto | always | never } > + Default: auto > + > + auto - Hash pointers unless slab_debug is enabled. > + always - Always hash pointers (even if slab_debug is > + enabled). > + never - Never hash pointers. This option should only > + be specified when debugging the kernel. Do > + not use on production kernels. The boot > + param "no_hash_pointers" is an alias for > + this mode. So on production one would want hash pointers (i.e. auto/always), right? Anyway, the wording LGTM. Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com> PS: I'm confused on unspoken convention that anything but otherwise specified is suitable for production, as I'm expecting explicit version deployment (e.g. use <insert something> version instead of don't use <otherwise>).
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. > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -60,6 +60,20 @@ > bool no_hash_pointers __ro_after_init; > EXPORT_SYMBOL_GPL(no_hash_pointers); > > +/* > + * Hashed pointers policy selected by "hash_pointers=..." boot param > + * > + * `auto` - Hashed pointers enabled unless disabled by slub_debug_enabled=true > + * `always` - Hashed pointers enabled unconditionally > + * `never` - Hashed pointers disabled unconditionally > + */ > +enum hash_pointers_policy { > + HASH_PTR_AUTO = 0, > + HASH_PTR_ALWAYS, > + HASH_PTR_NEVER > +}; > +static enum hash_pointers_policy hash_pointers_mode __initdata; > + > noinline > static unsigned long long simple_strntoull(const char *startp, char **endp, unsigned int base, size_t max_chars) > { > @@ -2271,12 +2285,13 @@ char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr, > return resource_string(buf, end, ptr, spec, fmt); > } > > -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 would suggest to use a generic names and allow to call it more times, something like: /** * hash_pointers_update() - update the decision whether to hash * printed pointers * @auto_disable: Disable hashing in auto mode * * The function allows to disable hashing printed pointers either * when the global mode is HASH_PTR_NEVER or when the caller * wants to disable it and the mode is HASH_PTR_AUTO. */ void __init hash_pointers_update(bool auto_disable) { bool disable_hashing = false; switch(hash_pointers_mode) { case HASH_PTR_AUTO: disable_hashing = auto_disable; break; case HASH_PTR_ALWAYS: disable_hashing = true; break; case HASH_PTR_NEVER: if (no_hash_pointers) { pr_warn("Pointers were temporary printed without hashing. Force hashing again.\n"); no_hash_pointers = false; } break; default: pr_warn("Unknown hash_pointers mode '%d' specified; assuming auto.\n", hash_pointers_mode); disable_hashing = auto_disable; } /* Nope when no change requested. */ if (no_hash_pointers || !disable_hashing) return; no_hash_pointers = true; pr_warn("**********************************************************\n"); pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"); pr_warn("** **\n"); pr_warn("** This system shows unhashed kernel memory addresses **\n"); pr_warn("** via the console, logs, and other interfaces. This **\n"); pr_warn("** might reduce the security of your system. **\n"); pr_warn("** **\n"); pr_warn("** If you see this message and you are not debugging **\n"); pr_warn("** the kernel, report this immediately to your system **\n"); pr_warn("** administrator! **\n"); pr_warn("** **\n"); pr_warn("** Use hash_pointers=always to force this mode off **\n"); pr_warn("** **\n"); pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"); pr_warn("**********************************************************\n"); } > @@ -2289,11 +2304,39 @@ int __init no_hash_pointers_enable(char *str) > pr_warn("** the kernel, report this immediately to your system **\n"); > pr_warn("** administrator! **\n"); > pr_warn("** **\n"); > + pr_warn("** Use hash_pointers=always to force this mode off **\n"); > + pr_warn("** **\n"); > pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"); > pr_warn("**********************************************************\n"); > +} > + > +static int __init hash_pointers_mode_parse(char *str) > +{ > + if (!str) { > + pr_warn("Hash pointers mode empty; falling back to auto.\n"); > + hash_pointers_mode = HASH_PTR_AUTO; > + } else if (strncmp(str, "auto", 4) == 0) { > + pr_info("Hash pointers mode set to auto.\n"); > + hash_pointers_mode = HASH_PTR_AUTO; > + } else if (strncmp(str, "never", 5) == 0) { > + pr_info("Hash pointers mode set to never.\n"); > + hash_pointers_mode = HASH_PTR_NEVER; > + } else if (strncmp(str, "always", 6) == 0) { > + pr_info("Hash pointers mode set to always.\n"); > + hash_pointers_mode = HASH_PTR_ALWAYS; This mode is not handled anywhere, see below. > + } else { > + pr_warn("Unknown hash_pointers mode '%s' specified; assuming auto.\n", str); > + hash_pointers_mode = HASH_PTR_AUTO; > + } We might handle HASH_PTR_ALWAYS by calling: hash_pointers_update(false); > return 0; > } > +early_param("hash_pointers", hash_pointers_mode_parse); > + > +static int __init no_hash_pointers_enable(char *str) > +{ > + return hash_pointers_mode_parse("never"); > +} > early_param("no_hash_pointers", no_hash_pointers_enable); > > /* Best Regards, Petr
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!
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 76e538c77e31..d0fd9c745db9 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1798,6 +1798,27 @@ backtraces on all cpus. Format: 0 | 1 + hash_pointers= + [KNL,EARLY] + By default, when pointers are printed to the console + or buffers via the %p format string, that pointer is + "hashed", i.e. obscured by hashing the pointer value. + This is a security feature that hides actual kernel + addresses from unprivileged users, but it also makes + debugging the kernel more difficult since unequal + pointers can no longer be compared. The choices are: + Format: { auto | always | never } + Default: auto + + auto - Hash pointers unless slab_debug is enabled. + always - Always hash pointers (even if slab_debug is + enabled). + never - Never hash pointers. This option should only + be specified when debugging the kernel. Do + not use on production kernels. The boot + param "no_hash_pointers" is an alias for + this mode. + hashdist= [KNL,NUMA] Large hashes allocated during boot are distributed across NUMA nodes. Defaults on for 64-bit NUMA, off otherwise. @@ -4120,18 +4141,7 @@ no_hash_pointers [KNL,EARLY] - Force pointers printed to the console or buffers to be - unhashed. By default, when a pointer is printed via %p - format string, that pointer is "hashed", i.e. obscured - by hashing the pointer value. This is a security feature - that hides actual kernel addresses from unprivileged - users, but it also makes debugging the kernel more - difficult since unequal pointers can no longer be - compared. However, if this command-line option is - specified, then all normal pointers will have their true - value printed. This option should only be specified when - debugging the kernel. Please do not use on production - kernels. + Alias for "hash_pointers=never". nohibernate [HIBERNATION] Disable hibernation and resume. diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h index 51cab2def9ec..521bb2cd2648 100644 --- a/include/linux/sprintf.h +++ b/include/linux/sprintf.h @@ -22,7 +22,7 @@ __scanf(2, 0) int vsscanf(const char *, const char *, va_list); /* These are for specific cases, do not use without real need */ extern bool no_hash_pointers; -int no_hash_pointers_enable(char *str); +void hash_pointers_finalize(bool slub_debug); /* Used for Rust formatting ('%pA') */ char *rust_fmt_argument(char *buf, char *end, const void *ptr); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 01699852f30c..1762cbbe1f5d 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -60,6 +60,20 @@ bool no_hash_pointers __ro_after_init; EXPORT_SYMBOL_GPL(no_hash_pointers); +/* + * Hashed pointers policy selected by "hash_pointers=..." boot param + * + * `auto` - Hashed pointers enabled unless disabled by slub_debug_enabled=true + * `always` - Hashed pointers enabled unconditionally + * `never` - Hashed pointers disabled unconditionally + */ +enum hash_pointers_policy { + HASH_PTR_AUTO = 0, + HASH_PTR_ALWAYS, + HASH_PTR_NEVER +}; +static enum hash_pointers_policy hash_pointers_mode __initdata; + noinline static unsigned long long simple_strntoull(const char *startp, char **endp, unsigned int base, size_t max_chars) { @@ -2271,12 +2285,13 @@ char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr, return resource_string(buf, end, ptr, spec, fmt); } -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"); @@ -2289,11 +2304,39 @@ int __init no_hash_pointers_enable(char *str) pr_warn("** the kernel, report this immediately to your system **\n"); pr_warn("** administrator! **\n"); pr_warn("** **\n"); + pr_warn("** Use hash_pointers=always to force this mode off **\n"); + pr_warn("** **\n"); pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"); pr_warn("**********************************************************\n"); +} + +static int __init hash_pointers_mode_parse(char *str) +{ + if (!str) { + pr_warn("Hash pointers mode empty; falling back to auto.\n"); + hash_pointers_mode = HASH_PTR_AUTO; + } else if (strncmp(str, "auto", 4) == 0) { + pr_info("Hash pointers mode set to auto.\n"); + hash_pointers_mode = HASH_PTR_AUTO; + } else if (strncmp(str, "never", 5) == 0) { + pr_info("Hash pointers mode set to never.\n"); + hash_pointers_mode = HASH_PTR_NEVER; + } else if (strncmp(str, "always", 6) == 0) { + pr_info("Hash pointers mode set to always.\n"); + hash_pointers_mode = HASH_PTR_ALWAYS; + } else { + pr_warn("Unknown hash_pointers mode '%s' specified; assuming auto.\n", str); + hash_pointers_mode = HASH_PTR_AUTO; + } return 0; } +early_param("hash_pointers", hash_pointers_mode_parse); + +static int __init no_hash_pointers_enable(char *str) +{ + return hash_pointers_mode_parse("never"); +} early_param("no_hash_pointers", no_hash_pointers_enable); /* diff --git a/mm/slub.c b/mm/slub.c index b46f87662e71..f3d61b330a76 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -6314,9 +6314,8 @@ void __init kmem_cache_init(void) if (debug_guardpage_minorder()) slub_max_order = 0; - /* Print slub debugging pointers without hashing */ - if (__slub_debug_enabled()) - no_hash_pointers_enable(NULL); + /* Inform pointer hashing choice about slub debugging state. */ + hash_pointers_finalize(__slub_debug_enabled()); kmem_cache_node = &boot_kmem_cache_node; kmem_cache = &boot_kmem_cache;
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". Link: https://github.com/KSPP/linux/issues/368 [1] Fixes: 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled") Co-developed-by: Sergio Perez Gonzalez <sperezglz@gmail.com> Signed-off-by: Sergio Perez Gonzalez <sperezglz@gmail.com> Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Petr Mladek <pmladek@suse.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Roman Gushchin <roman.gushchin@linux.dev> Cc: Harry Yoo <harry.yoo@oracle.com> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Tamir Duberstein <tamird@gmail.com> Cc: Miguel Ojeda <ojeda@kernel.org> Cc: Alice Ryhl <aliceryhl@google.com> Cc: linux-doc@vger.kernel.org Cc: linux-mm@kvack.org --- .../admin-guide/kernel-parameters.txt | 34 ++++++++----- include/linux/sprintf.h | 2 +- lib/vsprintf.c | 51 +++++++++++++++++-- mm/slub.c | 5 +- 4 files changed, 72 insertions(+), 20 deletions(-)