[PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

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

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Kees Cook]

On Tue, Feb 02, 2021 at 02:18:46PM -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.

Linus has expressly said "no" to things like this in the past:
https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/

-Kees

> 
> 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
> 

-- 
Kees Cook

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

On 2/2/21 3:52 PM, Kees Cook wrote:
>> 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.

> Linus has expressly said "no" to things like this in the past:
> https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/
Maybe I misunderstood, but I thought this is what Vlastimil, Petr, 
Sergey, John, and Steven asked for.

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Steven Rostedt]

On Tue, 2 Feb 2021 16:19:20 -0600
Timur Tabi <timur@kernel.org> wrote:

> On 2/2/21 3:52 PM, Kees Cook wrote:
> >> 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.  
> 
> > Linus has expressly said "no" to things like this in the past:
> > https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/  
> Maybe I misunderstood, but I thought this is what Vlastimil, Petr, 
> Sergey, John, and Steven asked for.

Maybe Linus changed his mind since then?


  "I also suspect that everybody has already accepted that KASLR isn't
   really working locally anyway (due to all the hw leak models with
   cache and TLB timing), so anybody who can look at kernel messages
   already probably could figure most of those things out."

 https://lore.kernel.org/r/CAHk-=wjnEV2E6vCRxv5S5m27iOjHeVWNbfK=JV8qxot4Do-FgA@mail.gmail.com


-- Steve

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Linus Torvalds]

On Tue, Feb 2, 2021 at 2:34 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>   "I also suspect that everybody has already accepted that KASLR isn't
>    really working locally anyway (due to all the hw leak models with
>    cache and TLB timing), so anybody who can look at kernel messages
>    already probably could figure most of those things out."

Honestly, if you have to pass a kernel command line, and there's a big
notice in the kernel messages about this, I no longer care.

Because it means that people who _do_ care will know about it.

But I don't want it to be a kernel config option - if you do
debugging, and you want unhidden pointers, you can add it to the
kernel command line and make sure it's *your* choice and not some
random kernel config by somebody else (ie distro).

And yes, my opinion is that KASRL really only works remotely anyway. I
think we might as well accept that as a fact, and that it's unlikely
that hardware will be fixed in general, even if on _some_ hardware
might make it work better than it works in general.

Instead of fighting windmills, accept that KASRL is dead locally for
the "wide access" cases (ie not necessarily just "shell access", but
"local JIT of uncontrolled code"), but do it because the remote case
still matters, and because a lot of local accesses are fairly
constrained in that they do *not* give random code execution to the
local users (but that "fairly constrained" presumably also generally
means that they can't do dmesg).

             Linus

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Kees Cook]

On Tue, Feb 02, 2021 at 02:51:00PM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2021 at 2:34 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >   "I also suspect that everybody has already accepted that KASLR isn't
> >    really working locally anyway (due to all the hw leak models with
> >    cache and TLB timing), so anybody who can look at kernel messages
> >    already probably could figure most of those things out."
> 
> Honestly, if you have to pass a kernel command line, and there's a big
> notice in the kernel messages about this, I no longer care.

Okay, cool; it's fine by me too. I prefer this kind of "boot into debug
mode" switch to having lots of %px scattered around in questionable
places. :)

I will update the %p deprecation docs.

-- 
Kees Cook

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]

On Tue 2021-02-02 14:18:46, 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.

Yes please. I needed to see pointers for debugging, and seeing hashed
pointers is nasty. Having to patch C code to be able to develop... is
bad.

> +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> +	pr_warn("** compromise security on your system.                  **\n");

This is lies, right? And way too verbose.

									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Steven Rostedt]

On Thu, 4 Feb 2021 21:48:35 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> > +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> > +	pr_warn("** compromise security on your system.                  **\n");  
> 
> This is lies, right? And way too verbose.

Not really. More of an exaggeration than a lie. And the verbosity is to
make sure it's noticed by those that shouldn't have it set. This works well
for keeping trace_printk() out of production kernels. Why do you care
anyway, you are just debugging it, and it shouldn't trigger any bug reports
on testing infrastructure. That's why I like the notice. It gets the job
done of keeping people from using things they shouldn't be using, and
doesn't cause testing failures that a WARN_ON would.

-- Steve

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

Hi!

> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > > +	pr_warn("** Kernel memory addresses are exposed, which may       **\n");
> > > +	pr_warn("** compromise security on your system.                  **\n");  
> > 
> > This is lies, right? And way too verbose.
> 
> Not really. More of an exaggeration than a lie. And the verbosity is
> to

Well... security is _not_ compromised but robustness against kernel
bugs is reduced. It should not exaggerate.

> make sure it's noticed by those that shouldn't have it set. This works well
> for keeping trace_printk() out of production kernels. Why do you
> care

So if we want people to see it, we up the severity, right? Like
pr_err()... Distro kernels have quiet, anyway...

Lets take a look for what we say for _real_ problems:

[    0.544757] Spectre V1 : Mitigation: usercopy/swapgs barriers and
__user pointer sanitiza
tion
[    0.544876] Spectre V2 : Mitigation: Full generic retpoline
[    0.544961] Spectre V2 : Spectre v2 / SpectreRSB mitigation:
Filling RSB on context switc
h
[    0.545064] L1TF: System has more than MAX_PA/2 memory. L1TF
mitigation not effective.
[    0.545163] L1TF: You may make it effective by booting the kernel
with mem=2147483648 par
ameter.
[    0.545281] L1TF: However, doing so will make a part of your RAM
unusable.
[    0.545374] L1TF: Reading
https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html
might help you decide.

This machine is insecure. Yet I don't see ascii-art *** all around..

"Kernel memory addresses are exposed, which is bad for security."
would be quite enough, I'd say...

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

On 2/4/21 3:49 PM, Pavel Machek wrote:
> This machine is insecure. Yet I don't see ascii-art *** all around..
> 
> "Kernel memory addresses are exposed, which is bad for security."

I'll use whatever wording everyone can agree on, but I really don't see 
much difference between "which may compromise security on your system" 
and "which is bad for security".  "may compromise" doesn't see any more 
alarmist than "bad".  Frankly, "bad" is a very generic term.

I think the reason behind the large banner has less to do how insecure 
the system is, and more about making sure vendors and sysadmins don't 
enable it by default everywhere.

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Steven Rostedt]

On Thu, 4 Feb 2021 22:49:44 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> This machine is insecure. Yet I don't see ascii-art *** all around..
> 
> "Kernel memory addresses are exposed, which is bad for security."
> would be quite enough, I'd say...

Well, the alternative is that you go back to patching your own kernel, and
we drop this patch altogether.

The compromise was to add this notice, to make sure that this doesn't get
set normally.

Changing the wording is fine, but to keep the option from being "forgotten
about" by a indiscrete message isn't going to fly.

-- Steve
 

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Steven Rostedt]

On Thu, 4 Feb 2021 15:59:21 -0600
Timur Tabi <timur@kernel.org> wrote:

> I think the reason behind the large banner has less to do how insecure 
> the system is, and more about making sure vendors and sysadmins don't 
> enable it by default everywhere.

+100

-- Steve

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Thu 2021-02-04 15:59:21, Timur Tabi wrote:
> On 2/4/21 3:49 PM, Pavel Machek wrote:
> >This machine is insecure. Yet I don't see ascii-art *** all around..
> >
> >"Kernel memory addresses are exposed, which is bad for security."
> 
> I'll use whatever wording everyone can agree on, but I really don't see much
> difference between "which may compromise security on your system" and "which
> is bad for security".  "may compromise" doesn't see any more alarmist than
> "bad".  Frankly, "bad" is a very generic term.

Well, I agree that "bad" is vague.... but original wording is simply
untrue, as printing addresses decreases robustness but can't introduce
security problem on its own.

Being alarmist is not my complaint; being untrue is.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Kees Cook]

On Thu, Feb 04, 2021 at 11:11:43PM +0100, Pavel Machek wrote:
> On Thu 2021-02-04 15:59:21, Timur Tabi wrote:
> > On 2/4/21 3:49 PM, Pavel Machek wrote:
> > >This machine is insecure. Yet I don't see ascii-art *** all around..
> > >
> > >"Kernel memory addresses are exposed, which is bad for security."
> > 
> > I'll use whatever wording everyone can agree on, but I really don't see much
> > difference between "which may compromise security on your system" and "which
> > is bad for security".  "may compromise" doesn't see any more alarmist than
> > "bad".  Frankly, "bad" is a very generic term.
> 
> Well, I agree that "bad" is vague.... but original wording is simply
> untrue, as printing addresses decreases robustness but can't introduce
> security problem on its own.
> 
> Being alarmist is not my complaint; being untrue is.

It's just semantics. Printing addresses DOES weaken the security of a
system, especially when we know attackers have and do use stuff from dmesg
to tune their attacks. How about "reduces the security of your system"?

-- 
Kees Cook

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Timur Tabi]

On 2/4/21 4:17 PM, Kees Cook wrote:
> It's just semantics. Printing addresses DOES weaken the security of a
> system, especially when we know attackers have and do use stuff from dmesg
> to tune their attacks. How about "reduces the security of your system"?

I think we're bikeshedding now, but I can replace "compromise" with 
"reduce".

"Kernel memory addresses are exposed, which may reduce the security of 
your system."

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Thu 2021-02-04 14:17:13, Kees Cook wrote:
> On Thu, Feb 04, 2021 at 11:11:43PM +0100, Pavel Machek wrote:
> > On Thu 2021-02-04 15:59:21, Timur Tabi wrote:
> > > On 2/4/21 3:49 PM, Pavel Machek wrote:
> > > >This machine is insecure. Yet I don't see ascii-art *** all around..
> > > >
> > > >"Kernel memory addresses are exposed, which is bad for security."
> > > 
> > > I'll use whatever wording everyone can agree on, but I really don't see much
> > > difference between "which may compromise security on your system" and "which
> > > is bad for security".  "may compromise" doesn't see any more alarmist than
> > > "bad".  Frankly, "bad" is a very generic term.
> > 
> > Well, I agree that "bad" is vague.... but original wording is simply
> > untrue, as printing addresses decreases robustness but can't introduce
> > security problem on its own.
> > 
> > Being alarmist is not my complaint; being untrue is.
> 
> It's just semantics. Printing addresses DOES weaken the security of a
> system, especially when we know attackers have and do use stuff from dmesg
> to tune their attacks. How about "reduces the security of your system"?

"reduces" sounds okay to me.

You should not have attackers on your system. That reduces your security.

You should not have users reading dmesg. Again that reduces your
security.

You should not have bugs in your kernel. That reduces your security.

But you really can't have attackers patching your kernel. That
compromises your security completely.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

On Thu 2021-02-04 23:51:36, Pavel Machek wrote:
> On Thu 2021-02-04 14:17:13, Kees Cook wrote:
> > On Thu, Feb 04, 2021 at 11:11:43PM +0100, Pavel Machek wrote:
> > > On Thu 2021-02-04 15:59:21, Timur Tabi wrote:
> > > > On 2/4/21 3:49 PM, Pavel Machek wrote:
> > > > >This machine is insecure. Yet I don't see ascii-art *** all around..
> > > > >
> > > > >"Kernel memory addresses are exposed, which is bad for security."
> > > > 
> > > > I'll use whatever wording everyone can agree on, but I really don't see much
> > > > difference between "which may compromise security on your system" and "which
> > > > is bad for security".  "may compromise" doesn't see any more alarmist than
> > > > "bad".  Frankly, "bad" is a very generic term.
> > > 
> > > Well, I agree that "bad" is vague.... but original wording is simply
> > > untrue, as printing addresses decreases robustness but can't introduce
> > > security problem on its own.
> > > 
> > > Being alarmist is not my complaint; being untrue is.
> > 
> > It's just semantics. Printing addresses DOES weaken the security of a
> > system, especially when we know attackers have and do use stuff from dmesg
> > to tune their attacks. How about "reduces the security of your system"?
> 
> "reduces" sounds okay to me.
> 
> You should not have attackers on your system. That reduces your security.
> 
> You should not have users reading dmesg. Again that reduces your
> security.
> 
> You should not have bugs in your kernel. That reduces your security.

Oh, and you really should not run modern, out-of-order CPU. That
significantly reduces your security.

Yet we have documentation stating that my machine is secure:

   The Linux kernel contains a mitigation for this attack vector, PTE
      inversion, which is permanently enabled and has no performance
      impact. The kernel ensures that the address bits of PTEs, which
      are not marked present, never point to cacheable physical memory
      space.

   A system with an up to date kernel is protected against attacks
   from malicious user space applications.

when it is not:

[    0.545064] L1TF: System has more than MAX_PA/2 memory. L1TF
mitigation not effective.
[    0.545163] L1TF: You may make it effective by booting the kernel
with mem=2147483648 parameter.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]