linux-kernel - Re: [RFC][PATCH] tracing, printk: Force no hashing when trace

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180403170612.7b11fc41@gandalf.local.home>
Date:   Tue, 3 Apr 2018 17:06:12 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "Tobin C. Harding" <me@...in.cc>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Laight <David.Laight@...lab.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Petr Mladek <pmladek@...e.com>
Subject: Re: [RFC][PATCH] tracing, printk: Force no hashing when
 trace_printk() is used

On Tue, 3 Apr 2018 13:07:58 -0700
Kees Cook <keescook@...omium.org> wrote:

> On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> > Both trace_debug is set and kptr_restrict is set to zero in the same
> > code that produces the above banner. This will allow trace_printk() to
> > not be affected by security code, as trace_printk() should never be run
> > on a machine that needs security of this kind.  
> 
> While I think it'd be nice to have a boot-time knob for this (a debate
> that was unsuccessful in earlier threads), I remain skeptical of
> having a _runtime_ knob for this, as then it becomes a target (and
> yes, there are plenty of targets, but why add another).
> 
> If this was __ro_after_init, maybe that'd be nicer. CONFIG_TRACING=y

Well, then of course this would need a check to keep modules from
setting it. But I think I know of a nice alternative.


> is used everywhere, so this is really just the whole knob debate over
> again. Instead, I've been following Linus's distillation of %p usage
> in the kernel:
> 
> http://lkml.kernel.org/r/CA+55aFwQEd_d40g4mUCSsVRZzrFPUJt74vc6PPpb675hYNXcKw@mail.gmail.com

Remember, this isn't a printk() that hangs around for production. I was
debugging code that modified pointers, and I wanted to make sure that
the pointer arithmetic was correct (it wasn't), and randomizing the
output made my prints useless.

If you are concerned about attack surface, I could make it a bit more
difficult to tweak by malicious software. What about the patch below?
It would be much more difficult to modify this knob from an attack
vector.

-- Steve

diff --git a/include/linux/printk.h b/include/linux/printk.h
index e9b603ee9953..b624493b3991 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -278,6 +278,7 @@ static inline void printk_safe_flush_on_panic(void)
 #endif
 
 extern int kptr_restrict;
+extern struct static_key trace_debug;
 
 extern asmlinkage void dump_stack(void) __cold;
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0f47e653ffd8..6c151d00848b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2864,6 +2864,10 @@ void trace_printk_init_buffers(void)
 
 	buffers_allocated = 1;
 
+	/* This is a debug kernel, allow pointers to be shown */
+	static_key_enable(&trace_debug);
+	kptr_restrict = 0;
+
 	/*
 	 * trace_printk_init_buffers() can be called by modules.
 	 * If that happens, then we need to start cmdline recording
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 89f8a4a4b770..c3d8eafecb39 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1345,6 +1345,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 int kptr_restrict __read_mostly;
+struct static_key trace_debug = STATIC_KEY_INIT_FALSE;
 
 static noinline_for_stack
 char *restricted_pointer(char *buf, char *end, const void *ptr,
@@ -1962,6 +1963,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return pointer_string(buf, end, ptr, spec);
 	}
 
+	/* When the kernel is in debugging mode, show all pointers */
+	if (static_key_false(&trace_debug))
+		return restricted_pointer(buf, end, ptr, spec);
+
 	/* default is to _not_ leak addresses, hash before printing */
 	return ptr_to_id(buf, end, ptr, spec);
 }