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@xxxxxxxxxxxx/

> Cc: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Cc: concord@xxxxxxxxxx
> Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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;
>