linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
@ 2021-02-02 21:36 Timur Tabi
  2021-02-03  3:24 ` Sergey Senozhatsky
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Timur Tabi @ 2021-02-02 21:36 UTC (permalink / raw)
  To: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, akinobu.mita

If the make-printk-non-secret command-line parameter is set, then
printk("%p") will print addresses as unhashed.  This is useful for
debugging purposes.

A large warning message is displayed if this option is enabled,
because unhashed addresses, while useful for debugging, exposes
kernel addresses which can be a security risk.

Signed-off-by: Timur Tabi <timur@kernel.org>
---
 lib/vsprintf.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..b9f87084afb0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,30 @@ 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 */
+static bool debug_never_hash_pointers __ro_after_init;
+
+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("** compromise security on 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 vendor!  **\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 +2321,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);
 }
 
 /*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-02 21:36 [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed Timur Tabi
@ 2021-02-03  3:24 ` Sergey Senozhatsky
  2021-02-03  3:30   ` Randy Dunlap
  2021-02-03  9:54 ` Petr Mladek
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Sergey Senozhatsky @ 2021-02-03  3:24 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, akinobu.mita

On (21/02/02 15:36), Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>

Looks good to me.

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03  3:24 ` Sergey Senozhatsky
@ 2021-02-03  3:30   ` Randy Dunlap
  0 siblings, 0 replies; 19+ messages in thread
From: Randy Dunlap @ 2021-02-03  3:30 UTC (permalink / raw)
  To: Sergey Senozhatsky, Timur Tabi
  Cc: Petr Mladek, Steven Rostedt, linux-kernel, vbabka, linux-mm,
	willy, akpm, torvalds, roman.fietze, keescook, john.ogness,
	akinobu.mita

On 2/2/21 7:24 PM, Sergey Senozhatsky wrote:
> On (21/02/02 15:36), Timur Tabi wrote:
>> If the make-printk-non-secret command-line parameter is set, then
>> printk("%p") will print addresses as unhashed.  This is useful for
>> debugging purposes.
>>
>> A large warning message is displayed if this option is enabled,
>> because unhashed addresses, while useful for debugging, exposes
>> kernel addresses which can be a security risk.
>>
>> Signed-off-by: Timur Tabi <timur@kernel.org>
> 
> Looks good to me.
> 
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> 	-ss

Acked-by: Randy Dunlap <rdunlap@infradead.org>

thanks.
-- 
~Randy

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-02 21:36 [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed Timur Tabi
  2021-02-03  3:24 ` Sergey Senozhatsky
@ 2021-02-03  9:54 ` Petr Mladek
  2021-02-03 13:31   ` Petr Mladek
  2021-02-05 10:59 ` Vlastimil Babka
  2021-02-09 21:59 ` Marco Elver
  3 siblings, 1 reply; 19+ messages in thread
From: Petr Mladek @ 2021-02-03  9:54 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Steven Rostedt, Sergey Senozhatsky, linux-kernel, vbabka,
	linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, akinobu.mita

On Tue 2021-02-02 15:36:33, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>
> ---
>  lib/vsprintf.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)

Please, add also entry into
Documentation/admin-guide/kernel-parameters.txt.

If we agree that the parameter is acceptable then let's make
it properly.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;
> +
> +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("** compromise security on 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 vendor!  **\n");

It is a boot parameter. So it should be "system administrtor" instead
of vendor.

Otherwise, it looks good to me.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03  9:54 ` Petr Mladek
@ 2021-02-03 13:31   ` Petr Mladek
  2021-02-03 18:58     ` Timur Tabi
  0 siblings, 1 reply; 19+ messages in thread
From: Petr Mladek @ 2021-02-03 13:31 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Steven Rostedt, Sergey Senozhatsky, linux-kernel, vbabka,
	linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed 2021-02-03 10:54:24, Petr Mladek wrote:
> On Tue 2021-02-02 15:36:33, Timur Tabi wrote:
> > If the make-printk-non-secret command-line parameter is set, then
> > printk("%p") will print addresses as unhashed.  This is useful for
> > debugging purposes.
> > 
> > A large warning message is displayed if this option is enabled,
> > because unhashed addresses, while useful for debugging, exposes
> > kernel addresses which can be a security risk.
> > 
> > Signed-off-by: Timur Tabi <timur@kernel.org>
> > ---
> >  lib/vsprintf.c | 34 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> Please, add also entry into
> Documentation/admin-guide/kernel-parameters.txt.

Adding Andy and Rasmus into CC. They are official vsprintf
co-maintainers and reviewers (MAINTAINERS file).

Also please make sure that lib/test_printf.c will work with
the new option.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 13:31   ` Petr Mladek
@ 2021-02-03 18:58     ` Timur Tabi
  2021-02-03 19:30       ` Andy Shevchenko
  2021-02-03 20:02       ` Kees Cook
  0 siblings, 2 replies; 19+ messages in thread
From: Timur Tabi @ 2021-02-03 18:58 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Steven Rostedt, Sergey Senozhatsky, linux-kernel, vbabka,
	linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On 2/3/21 7:31 AM, Petr Mladek wrote:
> Also please make sure that lib/test_printf.c will work with
> the new option.

As you suspected, it doesn't work:

[  206.966478] test_printf: loaded.
[  206.966528] test_printf: plain 'p' does not appear to be hashed
[  206.966740] test_printf: failed 1 out of 388 tests

What should I do about this?

On one hand, it is working as expected: %p is not hashed, and that 
should be a warning.

On the other hand, maybe test_printf should be aware of the command line 
parameter and test to make sure that %p is NOT hashed?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 18:58     ` Timur Tabi
@ 2021-02-03 19:30       ` Andy Shevchenko
  2021-02-03 20:02       ` Kees Cook
  1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2021-02-03 19:30 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, Rasmus Villemoes, akinobu.mita

On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote:
> On 2/3/21 7:31 AM, Petr Mladek wrote:
> > Also please make sure that lib/test_printf.c will work with
> > the new option.
> 
> As you suspected, it doesn't work:
> 
> [  206.966478] test_printf: loaded.
> [  206.966528] test_printf: plain 'p' does not appear to be hashed
> [  206.966740] test_printf: failed 1 out of 388 tests
> 
> What should I do about this?
> 
> On one hand, it is working as expected: %p is not hashed, and that should be
> a warning.
> 
> On the other hand, maybe test_printf should be aware of the command line
> parameter and test to make sure that %p is NOT hashed?

test_printf.c should be altered accordingly to avoid any failed test cases.
I.o.w. you need to have some kind of conditional there:

 if (kernel_cmdline_parameter_foo)
	expect bar
 else
	expect baz

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 18:58     ` Timur Tabi
  2021-02-03 19:30       ` Andy Shevchenko
@ 2021-02-03 20:02       ` Kees Cook
  2021-02-03 20:25         ` Steven Rostedt
  1 sibling, 1 reply; 19+ messages in thread
From: Kees Cook @ 2021-02-03 20:02 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote:
> On 2/3/21 7:31 AM, Petr Mladek wrote:
> > Also please make sure that lib/test_printf.c will work with
> > the new option.
> 
> As you suspected, it doesn't work:
> 
> [  206.966478] test_printf: loaded.
> [  206.966528] test_printf: plain 'p' does not appear to be hashed
> [  206.966740] test_printf: failed 1 out of 388 tests
> 
> What should I do about this?
> 
> On one hand, it is working as expected: %p is not hashed, and that should be
> a warning.
> 
> On the other hand, maybe test_printf should be aware of the command line
> parameter and test to make sure that %p is NOT hashed?

It seems like it'd be best for the test to fail, yes? It _is_ a problem
that %p is unhashed; it's just that the failure was intended.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 20:02       ` Kees Cook
@ 2021-02-03 20:25         ` Steven Rostedt
  2021-02-03 20:35           ` Kees Cook
  0 siblings, 1 reply; 19+ messages in thread
From: Steven Rostedt @ 2021-02-03 20:25 UTC (permalink / raw)
  To: Kees Cook
  Cc: Timur Tabi, Petr Mladek, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed, 3 Feb 2021 12:02:05 -0800
Kees Cook <keescook@chromium.org> wrote:

> On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote:
> > On 2/3/21 7:31 AM, Petr Mladek wrote:  
> > > Also please make sure that lib/test_printf.c will work with
> > > the new option.  
> > 
> > As you suspected, it doesn't work:
> > 
> > [  206.966478] test_printf: loaded.
> > [  206.966528] test_printf: plain 'p' does not appear to be hashed
> > [  206.966740] test_printf: failed 1 out of 388 tests
> > 
> > What should I do about this?
> > 
> > On one hand, it is working as expected: %p is not hashed, and that should be
> > a warning.
> > 
> > On the other hand, maybe test_printf should be aware of the command line
> > parameter and test to make sure that %p is NOT hashed?  
> 
> It seems like it'd be best for the test to fail, yes? It _is_ a problem
> that %p is unhashed; it's just that the failure was intended.
> 

I disagree.

With a big notice that all pointers of unhashed, I don't think we need to
print it failed when we expect it to fail.

If anything, skip the test and state:

  test_printf: hash test skipped because "make-printk-non-secret" is on the
  command line.

-- Steve

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 20:25         ` Steven Rostedt
@ 2021-02-03 20:35           ` Kees Cook
  2021-02-03 20:47             ` Steven Rostedt
  0 siblings, 1 reply; 19+ messages in thread
From: Kees Cook @ 2021-02-03 20:35 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Timur Tabi, Petr Mladek, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed, Feb 03, 2021 at 03:25:13PM -0500, Steven Rostedt wrote:
> On Wed, 3 Feb 2021 12:02:05 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
> > On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote:
> > > On 2/3/21 7:31 AM, Petr Mladek wrote:  
> > > > Also please make sure that lib/test_printf.c will work with
> > > > the new option.  
> > > 
> > > As you suspected, it doesn't work:
> > > 
> > > [  206.966478] test_printf: loaded.
> > > [  206.966528] test_printf: plain 'p' does not appear to be hashed
> > > [  206.966740] test_printf: failed 1 out of 388 tests
> > > 
> > > What should I do about this?
> > > 
> > > On one hand, it is working as expected: %p is not hashed, and that should be
> > > a warning.
> > > 
> > > On the other hand, maybe test_printf should be aware of the command line
> > > parameter and test to make sure that %p is NOT hashed?  
> > 
> > It seems like it'd be best for the test to fail, yes? It _is_ a problem
> > that %p is unhashed; it's just that the failure was intended.
> > 
> 
> I disagree.
> 
> With a big notice that all pointers of unhashed, I don't think we need to
> print it failed when we expect it to fail.
> 
> If anything, skip the test and state:
> 
>   test_printf: hash test skipped because "make-printk-non-secret" is on the
>   command line.

Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't
like. :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 20:35           ` Kees Cook
@ 2021-02-03 20:47             ` Steven Rostedt
  2021-02-03 21:56               ` Timur Tabi
  2021-02-04  9:36               ` Petr Mladek
  0 siblings, 2 replies; 19+ messages in thread
From: Steven Rostedt @ 2021-02-03 20:47 UTC (permalink / raw)
  To: Kees Cook
  Cc: Timur Tabi, Petr Mladek, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze,
	john.ogness, Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed, 3 Feb 2021 12:35:07 -0800
Kees Cook <keescook@chromium.org> wrote:

> > With a big notice that all pointers of unhashed, I don't think we need to
> > print it failed when we expect it to fail.
> > 
> > If anything, skip the test and state:
> > 
> >   test_printf: hash test skipped because "make-printk-non-secret" is on the
> >   command line.  
> 
> Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't
> like. :)

Is there any printing of the tests being done? Looks to me that the tests
only print something if they fail. Thus "skip" and "pass" are basically the
same (if "skip" is simply not to do the test).

I mean, we could simply have:


 static void __init
 plain(void)
 {
 	int err;
 
+	if (debug_never_hash_pointers)
+		return;



-- Steve

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 20:47             ` Steven Rostedt
@ 2021-02-03 21:56               ` Timur Tabi
  2021-02-03 22:38                 ` Steven Rostedt
  2021-02-04  9:36               ` Petr Mladek
  1 sibling, 1 reply; 19+ messages in thread
From: Timur Tabi @ 2021-02-03 21:56 UTC (permalink / raw)
  To: Steven Rostedt, Kees Cook
  Cc: Petr Mladek, Sergey Senozhatsky, linux-kernel, vbabka, linux-mm,
	willy, akpm, torvalds, roman.fietze, john.ogness,
	Andy Shevchenko, Rasmus Villemoes, akinobu.mita



On 2/3/21 2:47 PM, Steven Rostedt wrote:
>   static void __init
>   plain(void)
>   {
>   	int err;
>   
> +	if (debug_never_hash_pointers)
> +		return;

So, I have a stupid question.  What's the best way for test_printf.c to 
read the command line parameter?  Should I just do this in vsprintf.c:

/* Disable pointer hashing if requested */
static bool debug_never_hash_pointers __ro_after_init;
EXPORT_SYMBOL_GPL(debug_never_hash_pointers);

I'm not crazy about exporting this variable to other drivers.  It could 
be used to disable hashing by any driver.

AFAIK, the only command-line parameter code that works in drivers is 
module_parm, and that expects the module prefix on the command-line.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 21:56               ` Timur Tabi
@ 2021-02-03 22:38                 ` Steven Rostedt
  0 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2021-02-03 22:38 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Kees Cook, Petr Mladek, Sergey Senozhatsky, linux-kernel, vbabka,
	linux-mm, willy, akpm, torvalds, roman.fietze, john.ogness,
	Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed, 3 Feb 2021 15:56:20 -0600
Timur Tabi <timur@kernel.org> wrote:

> On 2/3/21 2:47 PM, Steven Rostedt wrote:
> >   static void __init
> >   plain(void)
> >   {
> >   	int err;
> >   
> > +	if (debug_never_hash_pointers)
> > +		return;  
> 
> So, I have a stupid question.  What's the best way for test_printf.c to 
> read the command line parameter?  Should I just do this in vsprintf.c:
> 
> /* Disable pointer hashing if requested */
> static bool debug_never_hash_pointers __ro_after_init;

It wont be static.

> EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
> 
> I'm not crazy about exporting this variable to other drivers.  It could 
> be used to disable hashing by any driver.

But it is set as "__ro_after_init". That is, every module will see it as
read only. IOW, they wont be able to modify it.

> 
> AFAIK, the only command-line parameter code that works in drivers is 
> module_parm, and that expects the module prefix on the command-line.

This is just a constant variable for others to see. The command line itself
is visible (see saved_command_line, it's even exported to modules in sparc).


-- Steve

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-03 20:47             ` Steven Rostedt
  2021-02-03 21:56               ` Timur Tabi
@ 2021-02-04  9:36               ` Petr Mladek
  1 sibling, 0 replies; 19+ messages in thread
From: Petr Mladek @ 2021-02-04  9:36 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Kees Cook, Timur Tabi, Sergey Senozhatsky, linux-kernel, vbabka,
	linux-mm, willy, akpm, torvalds, roman.fietze, john.ogness,
	Andy Shevchenko, Rasmus Villemoes, akinobu.mita

On Wed 2021-02-03 15:47:27, Steven Rostedt wrote:
> On Wed, 3 Feb 2021 12:35:07 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
> > > With a big notice that all pointers of unhashed, I don't think we need to
> > > print it failed when we expect it to fail.
> > > 
> > > If anything, skip the test and state:
> > > 
> > >   test_printf: hash test skipped because "make-printk-non-secret" is on the
> > >   command line.  
> > 
> > Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't
> > like. :)
> 
> Is there any printing of the tests being done? Looks to me that the tests
> only print something if they fail. Thus "skip" and "pass" are basically the
> same (if "skip" is simply not to do the test).

It prints the total number of tests done. It should not count the
skipped tests.

We actually print a warning when crng is not initialized. In this
case, the test passes because we actually check the value and it
is an expected one.

> I mean, we could simply have:
> 
> 
>  static void __init
>  plain(void)
>  {
>  	int err;
>  
> +	if (debug_never_hash_pointers)
> +		return;

I am not 100% sure. But this might work. Just please print a warning
about the tests are skipped.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-02 21:36 [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed Timur Tabi
  2021-02-03  3:24 ` Sergey Senozhatsky
  2021-02-03  9:54 ` Petr Mladek
@ 2021-02-05 10:59 ` Vlastimil Babka
  2021-02-05 18:25   ` Timur Tabi
  2021-02-09 21:59 ` Marco Elver
  3 siblings, 1 reply; 19+ messages in thread
From: Vlastimil Babka @ 2021-02-05 10:59 UTC (permalink / raw)
  To: Timur Tabi, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	linux-kernel, linux-mm, willy, akpm, torvalds, roman.fietze,
	keescook, john.ogness, akinobu.mita

On 2/2/21 10:36 PM, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>

Thanks a lot. Should this also affect %pK though? IIUC, there's currently no way
to achieve non-mangled %pK in all cases, even with the most permissive
kptr_restrict=1 setting:
- in IRQ, there's "pK-error" instead
- in a context of non-CAP_SYSLOG process, nulls are printed

Yes, neither should matter if %pK were only used for prints that generate
content of some kind of /proc file read by a CAP_SYSLOG process, but that
doesn't seem to be the case and there are %pK used for printing to dmesg too...

> ---
>  lib/vsprintf.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;
> +
> +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("** compromise security on 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 vendor!  **\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 +2321,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);
>  }
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-05 10:59 ` Vlastimil Babka
@ 2021-02-05 18:25   ` Timur Tabi
  2021-02-10  0:24     ` Kees Cook
  0 siblings, 1 reply; 19+ messages in thread
From: Timur Tabi @ 2021-02-05 18:25 UTC (permalink / raw)
  To: Vlastimil Babka, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	linux-kernel, linux-mm, willy, akpm, torvalds, roman.fietze,
	keescook, john.ogness, akinobu.mita



On 2/5/21 4:59 AM, Vlastimil Babka wrote:
> Thanks a lot. Should this also affect %pK though? IIUC, there's currently no way
> to achieve non-mangled %pK in all cases, even with the most permissive
> kptr_restrict=1 setting:
> - in IRQ, there's "pK-error" instead
> - in a context of non-CAP_SYSLOG process, nulls are printed

Hmmm..  I thought %pK prints an unhashed pointer when the user is root, 
at least in situations where the user can be known (e.g. during an ioctl 
call).

> Yes, neither should matter if %pK were only used for prints that generate
> content of some kind of /proc file read by a CAP_SYSLOG process, but that
> doesn't seem to be the case and there are %pK used for printing to dmesg too...

I thought about that.  On one hand, people who use %pK probably really 
wanted a hashed pointer printed.  On the other hand, I agree that %pK 
should not be used for dmesg prints.

I get the feeling that some (most?) people who use %pK don't really 
understand how it's supposed to be used.

I can extend make-printk-non-secret to %pK if everyone agrees.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-02 21:36 [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed Timur Tabi
                   ` (2 preceding siblings ...)
  2021-02-05 10:59 ` Vlastimil Babka
@ 2021-02-09 21:59 ` Marco Elver
  2021-02-09 22:15   ` Timur Tabi
  3 siblings, 1 reply; 19+ messages in thread
From: Marco Elver @ 2021-02-09 21:59 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, akinobu.mita, glider, andreyknvl

On Tue, Feb 02, 2021 at 03:36PM -0600, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed.  This is useful for
> debugging purposes.
> 
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
> 
> Signed-off-by: Timur Tabi <timur@kernel.org>
> ---
>  lib/vsprintf.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;

Would it be reasonable to make this non-static? Or somehow make it
possible to get this flag from other subsystems?

There are other places in the kernel that dump sensitive data such as
registers. We'd like to be able to use 'debug_never_hash_pointers' to
decide if our debugging tools can dump registers etc. What we really
need is info if the kernel is in debug mode and we can dump all kinds of
sensitive info; debug_never_hash_pointers is would be a good enough
proxy for that.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-09 21:59 ` Marco Elver
@ 2021-02-09 22:15   ` Timur Tabi
  0 siblings, 0 replies; 19+ messages in thread
From: Timur Tabi @ 2021-02-09 22:15 UTC (permalink / raw)
  To: Marco Elver
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, linux-kernel,
	vbabka, linux-mm, willy, akpm, torvalds, roman.fietze, keescook,
	john.ogness, akinobu.mita, glider, andreyknvl



On 2/9/21 3:59 PM, Marco Elver wrote:
> Would it be reasonable to make this non-static? Or somehow make it
> possible to get this flag from other subsystems?
> 
> There are other places in the kernel that dump sensitive data such as
> registers. We'd like to be able to use 'debug_never_hash_pointers' to
> decide if our debugging tools can dump registers etc. What we really
> need is info if the kernel is in debug mode and we can dump all kinds of
> sensitive info; debug_never_hash_pointers is would be a good enough
> proxy for that.

The next version of my patch (coming soon) will export the symbol.  It's 
intended for test_printf, but if you think it can be used by others, so 
much the better.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
  2021-02-05 18:25   ` Timur Tabi
@ 2021-02-10  0:24     ` Kees Cook
  0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-02-10  0:24 UTC (permalink / raw)
  To: Timur Tabi
  Cc: Vlastimil Babka, Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	linux-kernel, linux-mm, willy, akpm, torvalds, roman.fietze,
	john.ogness, akinobu.mita

On Fri, Feb 05, 2021 at 12:25:22PM -0600, Timur Tabi wrote:
> I can extend make-printk-non-secret to %pK if everyone agrees.

Let's just leave those alone. There is already a toggle for that in
/proc.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-02-10  1:58 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 21:36 [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed Timur Tabi
2021-02-03  3:24 ` Sergey Senozhatsky
2021-02-03  3:30   ` Randy Dunlap
2021-02-03  9:54 ` Petr Mladek
2021-02-03 13:31   ` Petr Mladek
2021-02-03 18:58     ` Timur Tabi
2021-02-03 19:30       ` Andy Shevchenko
2021-02-03 20:02       ` Kees Cook
2021-02-03 20:25         ` Steven Rostedt
2021-02-03 20:35           ` Kees Cook
2021-02-03 20:47             ` Steven Rostedt
2021-02-03 21:56               ` Timur Tabi
2021-02-03 22:38                 ` Steven Rostedt
2021-02-04  9:36               ` Petr Mladek
2021-02-05 10:59 ` Vlastimil Babka
2021-02-05 18:25   ` Timur Tabi
2021-02-10  0:24     ` Kees Cook
2021-02-09 21:59 ` Marco Elver
2021-02-09 22:15   ` Timur Tabi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).