Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all
 addresses as unhashed
To:     Timur Tabi <timur@kernel.org>, Petr Mladek <pmladek@suse.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        akpm@linux-foundation.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        roman.fietze@magna.com, Kees Cook <keescook@chromium.org>,
        John Ogness <john.ogness@linutronix.de>,
        akinobu.mita@gmail.com, glider@google.com,
        Andrey Konovalov <andreyknvl@google.com>,
        Marco Elver <elver@google.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Pavel Machek <pavel@ucw.cz>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
References: <20210210051814.845713-1-timur@kernel.org>
 <20210210051814.845713-4-timur@kernel.org>
From:   Vlastimil Babka <vbabka@suse.cz>
Message-ID: <9c6ed37a-aeae-7816-d204-ff752a05efdd@suse.cz>
Date:   Wed, 10 Feb 2021 12:03:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210210051814.845713-4-timur@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/10/21 6:18 AM, Timur Tabi wrote:
> If the make-printk-non-secret command line parameter is set, then
> printk("%p") will print pointers as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled.
> Unhashed pointers, while useful for debugging, expose kernel
> addresses which can be a security risk.
> 
> Also update test_printf to skip the hashed pointer tests if the
> command-line option is set.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>
> Acked-by: Petr Mladek <pmladek@suse.com>
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

> ---
>  .../admin-guide/kernel-parameters.txt         | 15 ++++++++
>  lib/test_printf.c                             |  8 ++++
>  lib/vsprintf.c                                | 38 ++++++++++++++++++-
>  3 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..6962379469e4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2613,6 +2613,21 @@
>  			different yeeloong laptops.
>  			Example: machtype=lemote-yeeloong-2f-7inch
>  
> +        make-printk-non-secret
> +			Force pointers printed to the console to be unhashed.
> +			By default, when a pointer is printed to the kernel
> +			console (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.  If this option is
> +			specified, then all normal pointers will have their
> +			true value printed.  Pointers printed via %pK may
> +			still be hashed.  This option should only be specified
> +			when debugging the kernel.  Please do not use on
> +			production kernels.
> +
>  	max_addr=nn[KMG]	[KNL,BOOT,ia64] All physical memory greater
>  			than or equal to this physical address is ignored.
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index ad2bcfa8caa1..b0b62d76e598 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
>  
> +extern bool debug_never_hash_pointers;
> +
>  static int __printf(4, 0) __init
>  do_test(int bufsize, const char *expect, int elen,
>  	const char *fmt, va_list ap)
> @@ -301,6 +303,12 @@ plain(void)
>  {
>  	int err;
>  
> +	if (debug_never_hash_pointers) {
> +		pr_warn("skipping plain 'p' tests");
> +		skipped_tests += 2;
> +		return;
> +	}
> +
>  	err = plain_hash();
>  	if (err) {
>  		pr_warn("plain 'p' does not appear to be hashed\n");
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..1296d9b0b328 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> +/* Disable pointer hashing if requested */
> +bool debug_never_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
> +
> +static int __init debug_never_hash_pointers_enable(char *str)
> +{
> +	debug_never_hash_pointers = true;
> +
> +	pr_warn("**********************************************************\n");
> +	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** All pointers that are printed to the console will    **\n");
> +	pr_warn("** be printed as unhashed.                              **\n");
> +	pr_warn("**                                                      **\n");
> +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> +	pr_warn("** 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("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
> +	pr_warn("**********************************************************\n");
> +
> +	return 0;
> +}
> +early_param("make-printk-non-secret", debug_never_hash_pointers_enable);
> +
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -2297,8 +2325,14 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		}
>  	}
>  
> -	/* default is to _not_ leak addresses, hash before printing */
> -	return ptr_to_id(buf, end, ptr, spec);
> +	/*
> +	 * default is to _not_ leak addresses, so hash before printing unless
> +	 * make-printk-non-secret is specified on the command line.
> +	 */
> +	if (unlikely(debug_never_hash_pointers))
> +		return pointer_string(buf, end, ptr, spec);
> +	else
> +		return ptr_to_id(buf, end, ptr, spec);
>  }
>  
>  /*
>