* [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
@ 2018-04-03 21:31 Steven Rostedt
2018-04-03 21:43 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2018-04-03 21:31 UTC (permalink / raw)
To: LKML
Cc: Tobin C. Harding, Linus Torvalds, Andrew Morton, David Laight,
Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Sergey Senozhatsky,
Kees Cook, Petr Mladek
From: Steven Rostedt (VMware) <rostedt@goodmis.org>
While debugging an issue I needed to see if the pointers were being
processed correctly with trace_printk() and after using "%p" and
triggering my bug and trace output, I was disappointed that all my
pointers were random garbage and didn't produce anything useful for me.
I had to rewrite all the trace_printk()s to use "%lx" instead.
As trace_printk() is not to be used for anything but debugging, and
this is enforced by printing in the dmesg:
**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** trace_printk() being used. Allocating extra memory. **
** **
** This means that this is a DEBUG kernel and it is **
** unsafe for production use. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your vendor! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************
on boot up if trace_printk() is used (or when a module is loaded that
uses trace_printk()), we can safely assume that the use of
trace_printk() is not going to be accidentally added to production code
(and if it is, they should be whacked with an overcooked spaghetti
noodle).
A static_key is added called "trace_debug" and if it is set, then %p will
not be hashed.
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.
Link: http://lkml.kernel.org/r/20180403154102.150b1be0@gandalf.local.home
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/printk.h | 1 +
kernel/trace/trace.c | 4 ++++
lib/vsprintf.c | 5 +++++
3 files changed, 10 insertions(+)
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);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 21:31 [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used Steven Rostedt
@ 2018-04-03 21:43 ` Kees Cook
2018-04-03 22:03 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2018-04-03 21:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Peter Zijlstra, Ingo Molnar, Thomas Gleixner,
Sergey Senozhatsky, Petr Mladek
On Tue, Apr 3, 2018 at 2:31 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> While debugging an issue I needed to see if the pointers were being
> processed correctly with trace_printk() and after using "%p" and
> triggering my bug and trace output, I was disappointed that all my
> pointers were random garbage and didn't produce anything useful for me.
> I had to rewrite all the trace_printk()s to use "%lx" instead.
>
> As trace_printk() is not to be used for anything but debugging, and
> this is enforced by printing in the dmesg:
>
> **********************************************************
> ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
> ** **
> ** trace_printk() being used. Allocating extra memory. **
> ** **
> ** This means that this is a DEBUG kernel and it is **
> ** unsafe for production use. **
> ** **
> ** If you see this message and you are not debugging **
> ** the kernel, report this immediately to your vendor! **
> ** **
> ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
> **********************************************************
>
> on boot up if trace_printk() is used (or when a module is loaded that
> uses trace_printk()), we can safely assume that the use of
> trace_printk() is not going to be accidentally added to production code
> (and if it is, they should be whacked with an overcooked spaghetti
> noodle).
>
> A static_key is added called "trace_debug" and if it is set, then %p will
> not be hashed.
Hrm, well, using a static key does make it weirder for an attacker to enable. :)
Whatever the case, if you can get Linus's Ack, sure. I would expect
he'd want you to change all the trace_printk()s to %px with
justifications, though.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 21:43 ` Kees Cook
@ 2018-04-03 22:03 ` Steven Rostedt
2018-04-04 2:34 ` Sergey Senozhatsky
0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2018-04-03 22:03 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Peter Zijlstra, Ingo Molnar, Thomas Gleixner,
Sergey Senozhatsky, Petr Mladek
On Tue, 3 Apr 2018 14:43:43 -0700
Kees Cook <keescook@chromium.org> wrote:
> > A static_key is added called "trace_debug" and if it is set, then %p will
> > not be hashed.
>
> Hrm, well, using a static key does make it weirder for an attacker to enable. :)
Not just weirder, much more difficult. static_keys are read only (code
segments). To enable them, the system makes the code portion rw updates
the code, then sets it back to ro.
>
> Whatever the case, if you can get Linus's Ack, sure. I would expect
Linus you OK with this?
> he'd want you to change all the trace_printk()s to %px with
> justifications, though.
What trace_printk()s do you want to change? They are throw away
functions. trace_printk() is not something that stays in the kernel.
It's added during debugging and then removed before submitting what you
are working on upstream. It's just annoying to have to use %px instead
of %p when debugging.
-- Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 22:03 ` Steven Rostedt
@ 2018-04-04 2:34 ` Sergey Senozhatsky
2018-04-04 17:13 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Senozhatsky @ 2018-04-04 2:34 UTC (permalink / raw)
To: Steven Rostedt
Cc: Kees Cook, LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Peter Zijlstra, Ingo Molnar, Thomas Gleixner,
Sergey Senozhatsky, Petr Mladek
On (04/03/18 18:03), Steven Rostedt wrote:
>
> > he'd want you to change all the trace_printk()s to %px with
> > justifications, though.
>
> What trace_printk()s do you want to change? They are throw away
> functions. trace_printk() is not something that stays in the kernel.
> It's added during debugging and then removed before submitting what you
> are working on upstream.
Seems that your patch also can fix up bpf_trace_printk() - it doesn't
even support %px, only hashed %p, which it passes to __trace_printk().
-ss
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 2:34 ` Sergey Senozhatsky
@ 2018-04-04 17:13 ` Steven Rostedt
2018-04-04 19:04 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2018-04-04 17:13 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Kees Cook, LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Peter Zijlstra, Ingo Molnar, Thomas Gleixner,
Petr Mladek
On Wed, 4 Apr 2018 11:34:55 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> On (04/03/18 18:03), Steven Rostedt wrote:
> >
> > > he'd want you to change all the trace_printk()s to %px with
> > > justifications, though.
> >
> > What trace_printk()s do you want to change? They are throw away
> > functions. trace_printk() is not something that stays in the kernel.
> > It's added during debugging and then removed before submitting what you
> > are working on upstream.
>
> Seems that your patch also can fix up bpf_trace_printk() - it doesn't
> even support %px, only hashed %p, which it passes to __trace_printk().
>
Crap, that should not work, but it does. I need to update the code to
prevent that from happening. I don't want any user space code to be
able to enable this.
Something like this will even prevent modules from disabling the printk
hash...
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2a1d01666dd2..c4af083742b0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2834,6 +2834,14 @@ static int alloc_percpu_trace_buffer(void)
static int buffers_allocated;
+/* Will become read only after boot */
+static const bool disable_printk_hash;
+
+static void set_printk_hash(bool val)
+{
+ *(bool *)&disable_printk_hash = val;
+}
+
void trace_printk_init_buffers(void)
{
if (buffers_allocated)
@@ -2864,9 +2872,11 @@ void trace_printk_init_buffers(void)
buffers_allocated = 1;
- /* This is a debug kernel, allow pointers to be shown */
- static_branch_enable(&trace_debug);
- kptr_restrict = 0;
+ if (disable_printk_hash) {
+ /* This is a debug kernel, allow pointers to be shown */
+ static_branch_enable(&trace_debug);
+ kptr_restrict = 0;
+ }
/*
* trace_printk_init_buffers() can be called by modules.
@@ -8456,10 +8466,13 @@ __init static int tracer_alloc_buffers(void)
if (!alloc_cpumask_var(&global_trace.tracing_cpumask, GFP_KERNEL))
goto out_free_buffer_mask;
+ /* Only trace_printk() in the core kernel can disable hashing */
+ set_printk_hash(true);
/* Only allocate trace_printk buffers if a trace_printk exists */
if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
/* Must be called before global_trace.buffer is allocated */
trace_printk_init_buffers();
+ set_printk_hash(false);
/* To save memory, keep the ring buffer size to its minimum */
if (ring_buffer_expanded)
-- Steve
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 17:13 ` Steven Rostedt
@ 2018-04-04 19:04 ` Linus Torvalds
2018-04-04 19:30 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2018-04-04 19:04 UTC (permalink / raw)
To: Steven Rostedt
Cc: Sergey Senozhatsky, Kees Cook, LKML, Tobin C. Harding,
Andrew Morton, David Laight, Peter Zijlstra, Ingo Molnar,
Thomas Gleixner, Petr Mladek
On Wed, Apr 4, 2018 at 10:13 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Something like this will even prevent modules from disabling the printk
> hash...
That still seems broken.
The *natural* thing to do would seem to be to tie the hash to the
printk state, kind of like the percpu buffers that safe_printk() and
friends use.
Modifying the hash global is fundamentally broken, since some problem
that happens *during* tracing - on another CPU entirely - would now
have the hashing disabled.
So at the *very* least this would need to be percpu logic, but even
that is honestly broken since an NMI might come in and want to printk
too.
Why don't you just use %px? That avoids all of these hacks.
So NAK on this stupid "enable and disable hashing that is
fundamentally broken" approach.
Linus
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 19:04 ` Linus Torvalds
@ 2018-04-04 19:30 ` Steven Rostedt
0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2018-04-04 19:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Sergey Senozhatsky, Kees Cook, LKML, Tobin C. Harding,
Andrew Morton, David Laight, Peter Zijlstra, Ingo Molnar,
Thomas Gleixner, Petr Mladek
On Wed, 4 Apr 2018 12:04:42 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> So at the *very* least this would need to be percpu logic, but even
> that is honestly broken since an NMI might come in and want to printk
> too.
>
> Why don't you just use %px? That avoids all of these hacks.
Just need to remember to do that. I guess with time it will become
second nature.
>
> So NAK on this stupid "enable and disable hashing that is
> fundamentally broken" approach.
>
OK. I'll save the change locally, and add it while debugging kernels.
ktest can do that for me ;-)
-- Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-04 19:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-03 21:31 [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used Steven Rostedt
2018-04-03 21:43 ` Kees Cook
2018-04-03 22:03 ` Steven Rostedt
2018-04-04 2:34 ` Sergey Senozhatsky
2018-04-04 17:13 ` Steven Rostedt
2018-04-04 19:04 ` Linus Torvalds
2018-04-04 19:30 ` Steven Rostedt
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).