Revert "slub: force on no_hash_pointers when slub_debug is enabled"
Subsystems (5)
PATCH 1/1 Revert "slub: force on no_hash_pointers when slub_debug is enabled"
This reverts commit 792702911f581f7793962fbeb99d5c3a1b28f4c3.
Linking no_hash_pointers() to slub_debug has had a chilling effect on using slub_debug features for security hardening, since system builders are forced to choose between redzoning and heap address location exposures. Instead, just require that the "no_hash_pointers" boot param needs to be used to expose pointers during slub_debug reports.
--- include/linux/kernel.h | 2 -- lib/vsprintf.c | 2 +- mm/slub.c | 4 ---- 3 files changed, 1 insertion(+), 7 deletions(-)
include/linux/kernel.h +0 -2
@@ -229,8 +229,6 @@ int sscanf(const char *, const char *, ...); extern __scanf(2, 0) int vsscanf(const char *, const char *, va_list); -extern int no_hash_pointers_enable(char *str); - extern int get_option(char **str, int *pint); extern char *get_options(const char *str, int nints, int *ints); extern unsigned long long memparse(const char *ptr, char **retptr);
lib/vsprintf.c +1 -1
@@ -2222,7 +2222,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,return widen_string (buf, buf - buf_start, end, spec); } -int __init no_hash_pointers_enable (char *str) +static int __init no_hash_pointers_enable (char *str) {if (no_hash_pointers)return 0 ;
mm/slub.c +0 -4
@@ -5005,10 +5005,6 @@ 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 ); - kmem_cache_node = &boot_kmem_cache_node; kmem_cache = &boot_kmem_cache;
Signature · 1 line
2.34.1
Quoted 8 lines · Kees Cook
This reverts commit
792702911f581f7793962fbeb99d5c3a1b28f4c3.Linking
no_hash_pointers()toslub_debughas had a chilling effect on usingslub_debugfeatures for security hardening, since system builders are forced to choose between redzoning and heap address location exposures. Instead, just require that the "no_hash_pointers" boot param needs to be used to expose pointers duringslub_debugreports.
Quoted 11 lines
Cc Vlastimil Babka <[email protected]>Cc Stephen Boyd <[email protected]>Cc Pekka Enberg <[email protected]>Cc David Rientjes <[email protected]>Cc Joonsoo Kim <[email protected]>Cc Petr Mladek <[email protected]>Signed-off-by Kees Cook <[email protected]>
in the commit message:
Quoted 9 lines
Obscuring the pointers that slub shows when debugging makes for some confusing slub debug messages:
Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
Those addresses are hashed for kernel security reasons. If we're trying to be secure with
slub_debugon the commandline we have some big problems given that we dump whole chunks of kernel memory to the kernel logs.
it dumps parts of kernel memory anyway and I'm not sure if slub_debug is supposed to be used for security hardening.
what about introducing new boot parameter like, slub_hardening, which does not print anything?
Quoted 33 lines · Hyeonggon Yoo
Quoted 8 lines · Kees Cook
This reverts commit
792702911f581f7793962fbeb99d5c3a1b28f4c3.Linking
no_hash_pointers()toslub_debughas had a chilling effect on usingslub_debugfeatures for security hardening, since system builders are forced to choose between redzoning and heap address location exposures. Instead, just require that the "no_hash_pointers" boot param needs to be used to expose pointers duringslub_debugreports.Quoted 11 lines
Cc Vlastimil Babka <[email protected]>Cc Stephen Boyd <[email protected]>Cc Pekka Enberg <[email protected]>Cc David Rientjes <[email protected]>Cc Joonsoo Kim <[email protected]>Cc Petr Mladek <[email protected]>Signed-off-by Kees Cook <[email protected]>in the commit message:
Quoted 9 lines
Obscuring the pointers that slub shows when debugging makes for some confusing slub debug messages:
Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17
Those addresses are hashed for kernel security reasons. If we're trying to be secure with
slub_debugon the commandline we have some big problems given that we dump whole chunks of kernel memory to the kernel logs.
Quoted 5 lines
it dumps parts of kernel memory anyway and I'm not sure if
slub_debugis supposed to be used for security hardening.what about introducing new boot parameter like,
slub_hardening, which does not print anything?
But it would be parsed for the same options? Redzoning, for example, is the common thing used for folks interested in detecting memory corruption attacks, etc.
Signature · 1 line
Kees Cook
Quoted 7 lines · Kees Cook
This reverts commit
792702911f581f7793962fbeb99d5c3a1b28f4c3.Linking
no_hash_pointers()toslub_debughas had a chilling effect on usingslub_debugfeatures for security hardening, since system builders are forced to choose between redzoning and heap address location exposures. Instead, just require that the "no_hash_pointers" boot param needs to be used to expose pointers duringslub_debugreports.
Searching lore made me find [1] and I like the more concrete changelog as well as updated documentation.
While it's convenient, it's probably indeed not slub's decision to enable no_hash_pointers. But Stephen also has a point in reply to [1]. If the data/address leak is indeed a concern, slub_debug will still print raw dumps as part of the reports, so will e.g. dump_page(), so hashing %p is not a complete solution and something more generic could be created for controlling prints that distinguishes hardening vs debugging?
[1]
https://lore.kernel.org/all/[email protected]/
Quoted 58 lines
Cc Vlastimil Babka <[email protected]>Cc Stephen Boyd <[email protected]>Cc Pekka Enberg <[email protected]>Cc David Rientjes <[email protected]>Cc Joonsoo Kim <[email protected]>Cc Petr Mladek <[email protected]>Signed-off-by Kees Cook <[email protected]>--- include/linux/kernel.h | 2 -- lib/vsprintf.c | 2 +- mm/slub.c | 4 ---- 3 files changed, 1 insertion(+), 7 deletions(-)include/linux/kernel.h +0
-2@@ -229,8 +229,6 @@ int sscanf(const char *, const char *, ...); extern __scanf(2, 0) int vsscanf(const char *, const char *, va_list); -extern int no_hash_pointers_enable(char *str); - extern int get_option(char **str, int *pint); extern char *get_options(const char *str, int nints, int *ints); extern unsigned long long memparse(const char *ptr, char **retptr);lib/vsprintf.c +1
-1@@ -2222,7 +2222,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,return widen_string (buf, buf - buf_start, end, spec); } -int __init no_hash_pointers_enable (char *str) +static int __init no_hash_pointers_enable (char *str) {if (no_hash_pointers)return 0 ;mm/slub.c +0
-4@@ -5005,10 +5005,6 @@ 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 ); - kmem_cache_node = &boot_kmem_cache_node; kmem_cache = &boot_kmem_cache;