| Message ID | 20210202201846.716915-1-timur@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lib/vsprintf: make-printk-non-secret printks all addresses as unhashed | expand |
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 >
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.
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
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
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.
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
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
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
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.
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
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
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
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"?
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."
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
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
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); } /*
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(-)