[PATCH] Revert "slub: force on no_hash_pointers when slub_debug is enabled"

[PATCH] Revert "slub: force on no_hash_pointers when slub_debug is enabled"

[Kees Cook]

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.

Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: concord@gentoo.org
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: linux-mm@kvack.org
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/kernel.h | 2 --
 lib/vsprintf.c         | 2 +-
 mm/slub.c              | 4 ----
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fe6efb24d151..e3d9d3879495 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index be71a03c936a..410b4a80a58a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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;
diff --git a/mm/slub.c b/mm/slub.c
index 13459c69095a..63f7337dd433 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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;
 
-- 
2.34.1

Re: [PATCH] Revert "slub: force on no_hash_pointers when slub_debug is enabled"

[Hyeonggon Yoo]

On Wed, Feb 08, 2023 at 11:47:17AM -0800, Kees Cook wrote:
> 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.
> 
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: concord@gentoo.org
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: linux-mm@kvack.org
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
> Signed-off-by: Kees Cook <keescook@chromium.org>

in the commit message:

> 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_debug on 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?

Re: [PATCH] Revert "slub: force on no_hash_pointers when slub_debug is enabled"

[Kees Cook]

On Wed, Feb 08, 2023 at 11:58:54PM +0000, Hyeonggon Yoo wrote:
> On Wed, Feb 08, 2023 at 11:47:17AM -0800, Kees Cook wrote:
> > 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.
> > 
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > Cc: concord@gentoo.org
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Petr Mladek <pmladek@suse.com>
> > Cc: linux-mm@kvack.org
> > Cc: stable@vger.kernel.org
> > Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> in the commit message:
> 
> > 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_debug on 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?

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.

-- 
Kees Cook

Re: [PATCH] Revert "slub: force on no_hash_pointers when slub_debug is enabled"

[Vlastimil Babka]

On 2/8/23 20:47, Kees Cook wrote:
> 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.

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/8e472c9e-2076-bc25-5912-8433adf7b579@arbitrary.ch/

> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: concord@gentoo.org
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: linux-mm@kvack.org
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/kernel.h | 2 --
>  lib/vsprintf.c         | 2 +-
>  mm/slub.c              | 4 ----
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index fe6efb24d151..e3d9d3879495 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -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);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index be71a03c936a..410b4a80a58a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -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;
> diff --git a/mm/slub.c b/mm/slub.c
> index 13459c69095a..63f7337dd433 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -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;
>