linux-hardening - Re: [PATCH] Revert "slub: force on no_hash_pointers when slub

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <046f0c76-f2b1-5df7-7c3e-76c0187da069@suse.cz>
Date:   Fri, 10 Feb 2023 09:44:11 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Kees Cook <keescook@...omium.org>
Cc:     Stephen Boyd <swboyd@...omium.org>, concord@...too.org,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Petr Mladek <pmladek@...e.com>, linux-mm@...ck.org,
        stable@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Christoph Lameter <cl@...ux.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>,
        Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        Bart Van Assche <bvanassche@....org>,
        Mikulas Patocka <mpatocka@...hat.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        Peter Gerber <peter@...itrary.ch>
Subject: Re: [PATCH] Revert "slub: force on no_hash_pointers when slub_debug
 is enabled"

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@...e.cz>
> Cc: Stephen Boyd <swboyd@...omium.org>
> Cc: concord@...too.org
> Cc: Pekka Enberg <penberg@...nel.org>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> Cc: Petr Mladek <pmladek@...e.com>
> Cc: linux-mm@...ck.org
> Cc: stable@...r.kernel.org
> Link: https://lore.kernel.org/lkml/202109200726.2EFEDC5@keescook/
> Signed-off-by: Kees Cook <keescook@...omium.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;
>