slab: Decouple slab_debug and no_hash_pointers
Subsystems (5)
PATCH 1/1 slab: Decouple slab_debug and no_hash_pointers
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".
---
Cc: Vlastimil Babka <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Harry Yoo <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Tamir Duberstein <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Cc: Alice Ryhl <[email protected]>
Cc: [email protected]
Cc: [email protected]
--- .../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(-)
Documentation/admin-guide/kernel-parameters.txt +22 -12
@@ -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.
include/linux/sprintf.h +1 -1
@@ -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);
lib/vsprintf.c +47 -4
76 lines not on this page · raw message
mm/slub.c +2 -3
@@ -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;
Signature · 1 line
2.34.1
Quoted 9 lines · Kees Cook
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".
Quoted 7 lines
This makes it possible to boot with "slab_debug=FPZ hash_pointers=always".
Fixes 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled")Co-developed-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Kees Cook <[email protected]>
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
Thanks!
Quoted 9 lines · Kees Cook
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".
Quoted 7 lines
This makes it possible to boot with "slab_debug=FPZ hash_pointers=always".
Fixes 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled")Co-developed-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Kees Cook <[email protected]>
Quoted 63 lines · Kees Cook
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".
Fixes 792702911f58 ("slub: force on no_hash_pointers when slub_debug is enabled")Co-developed-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Sergio Perez Gonzalez <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Cc: Vlastimil Babka <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Harry Yoo <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Tamir Duberstein <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Cc: Alice Ryhl <[email protected]>
Cc: [email protected]
Cc: [email protected]--- .../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(-)Documentation/admin-guide/kernel-parameters.txt +10
-0@@ -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 }
Documentation/admin-guide/kernel-parameters.txt line 1820
Quoted 10 lines
Documentation/admin-guide/kernel-parameters.txt +10
-0+ 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.
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>).
Signature · 1 line
An old man doll... just what I always wanted! - Clara
Petr Mladek
maintainerQuoted 10 lines · Kees Cook
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.
Quoted 31 lines
lib/vsprintf.c +15
-2@@ -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)
lib/vsprintf.c char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr, · line 2297
Quoted 10 lines
lib/vsprintf.c +4
-2-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");
}Quoted 14 lines
lib/vsprintf.c +8
-0@@ -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" );
lib/vsprintf.c int __init no_hash_pointers_enable(char *str) · line 2326
Quoted 10 lines
lib/vsprintf.c +10
-0+ 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.
lib/vsprintf.c int __init no_hash_pointers_enable(char *str) · line 2330
Quoted 4 lines
lib/vsprintf.c +4
-0+ }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);
lib/vsprintf.c int __init no_hash_pointers_enable(char *str) · line 2342
Quoted 11 lines
lib/vsprintf.c +6
-0return 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
Quoted 10 lines · Petr Mladek
Quoted 9 lines · Kees Cook
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".
Quoted 4 lines
Quoted 1 line
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.
Quoted 12 lines
Quoted 12 lines
lib/vsprintf.c +5
-4-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 ;
Quoted 6 lines
lib/vsprintf.c char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr, · line 2297
Quoted 2 lines
lib/vsprintf.c +0
-0pr_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!
Signature · 1 line
Kees Cook