* [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
@ 2018-04-03 19:41 Steven Rostedt
2018-04-03 20:07 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2018-04-03 19:41 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
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 new bool 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.
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/include/linux/printk.h b/include/linux/printk.h
index e9b603ee9953..7ef6c31d874a 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 bool trace_debug;
extern asmlinkage void dump_stack(void) __cold;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0f47e653ffd8..cb58cd4ee3a7 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 */
+ trace_debug = true;
+ 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..425644ceedcb 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;
+bool trace_debug __read_mostly;
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 (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);
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 19:41 [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used Steven Rostedt
@ 2018-04-03 20:07 ` Kees Cook
2018-04-03 21:06 ` Steven Rostedt
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2018-04-03 20:07 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 12:41 PM, Steven Rostedt <rostedt@goodmis.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
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
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 20:07 ` Kees Cook
@ 2018-04-03 21:06 ` Steven Rostedt
2018-04-03 21:43 ` Tobin C. Harding
2018-04-04 7:49 ` Peter Zijlstra
0 siblings, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2018-04-03 21:06 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 13:07:58 -0700
Kees Cook <keescook@chromium.org> wrote:
> On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt <rostedt@goodmis.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);
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 21:06 ` Steven Rostedt
@ 2018-04-03 21:43 ` Tobin C. Harding
2018-04-03 22:59 ` Steven Rostedt
2018-04-04 7:49 ` Peter Zijlstra
1 sibling, 1 reply; 9+ messages in thread
From: Tobin C. Harding @ 2018-04-03 21:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Kees Cook, LKML, Linus Torvalds, Andrew Morton, David Laight,
Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Sergey Senozhatsky,
Petr Mladek
On Tue, Apr 03, 2018 at 05:06:12PM -0400, Steven Rostedt wrote:
> On Tue, 3 Apr 2018 13:07:58 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
> > On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt <rostedt@goodmis.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);
> }
This uses the deprecated API Steve (I only know because I went to read
Documentation/static-keys.txt after seeing this patch).
Hope this helps,
Tobin.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 21:43 ` Tobin C. Harding
@ 2018-04-03 22:59 ` Steven Rostedt
0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2018-04-03 22:59 UTC (permalink / raw)
To: Tobin C. Harding
Cc: Kees Cook, LKML, Linus Torvalds, Andrew Morton, David Laight,
Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Sergey Senozhatsky,
Petr Mladek
On Wed, 4 Apr 2018 07:43:49 +1000
"Tobin C. Harding" <me@tobin.cc> wrote:
> > 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);
> > }
>
> This uses the deprecated API Steve (I only know because I went to read
> Documentation/static-keys.txt after seeing this patch).
Hmm, I've been involved with static keys since it was introduced. I
simply copied the code for the original use case (tracepoints). I
forgot that we are changing the name (this has been an ongoing bikeshed
for some time, I can't keep up. Better than the original suggestion
"really_unlikely()" ;)
I need to update tracepoints too.
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-03 21:06 ` Steven Rostedt
2018-04-03 21:43 ` Tobin C. Harding
@ 2018-04-04 7:49 ` Peter Zijlstra
2018-04-04 13:36 ` Steven Rostedt
2018-04-04 16:27 ` Kees Cook
1 sibling, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-04-04 7:49 UTC (permalink / raw)
To: Steven Rostedt
Cc: Kees Cook, LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Ingo Molnar, Thomas Gleixner, Sergey Senozhatsky,
Petr Mladek
On Tue, Apr 03, 2018 at 05:06:12PM -0400, Steven Rostedt wrote:
> 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.
Not if you build using clang, because that doesn't support asm-goto and
thus falls back to a simple runtime variable, which is exactly what Kees
didn't want.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 7:49 ` Peter Zijlstra
@ 2018-04-04 13:36 ` Steven Rostedt
2018-04-04 16:27 ` Kees Cook
1 sibling, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2018-04-04 13:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Kees Cook, LKML, Tobin C. Harding, Linus Torvalds, Andrew Morton,
David Laight, Ingo Molnar, Thomas Gleixner, Sergey Senozhatsky,
Petr Mladek
On Wed, 4 Apr 2018 09:49:27 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Apr 03, 2018 at 05:06:12PM -0400, Steven Rostedt wrote:
> > 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.
>
> Not if you build using clang, because that doesn't support asm-goto and
> thus falls back to a simple runtime variable, which is exactly what Kees
> didn't want.
Fix clang ;-)
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 7:49 ` Peter Zijlstra
2018-04-04 13:36 ` Steven Rostedt
@ 2018-04-04 16:27 ` Kees Cook
2018-04-04 16:52 ` Steven Rostedt
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2018-04-04 16:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, LKML, Tobin C. Harding, Linus Torvalds,
Andrew Morton, David Laight, Ingo Molnar, Thomas Gleixner,
Sergey Senozhatsky, Petr Mladek
On Wed, Apr 4, 2018 at 12:49 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Apr 03, 2018 at 05:06:12PM -0400, Steven Rostedt wrote:
>> 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.
>
> Not if you build using clang, because that doesn't support asm-goto and
> thus falls back to a simple runtime variable, which is exactly what Kees
> didn't want.
Nah, Clang will get asm-goto soon. I'm not worried about that. Besides
the "yay new target issue", I was concerned about Linus yelling about
seeing the knob added he specifically said he didn't want. :P
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
2018-04-04 16:27 ` Kees Cook
@ 2018-04-04 16:52 ` Steven Rostedt
0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2018-04-04 16:52 UTC (permalink / raw)
To: Kees Cook
Cc: Peter Zijlstra, LKML, Tobin C. Harding, Linus Torvalds,
Andrew Morton, David Laight, Ingo Molnar, Thomas Gleixner,
Sergey Senozhatsky, Petr Mladek
On Wed, 4 Apr 2018 09:27:10 -0700
Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 4, 2018 at 12:49 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, Apr 03, 2018 at 05:06:12PM -0400, Steven Rostedt wrote:
> >> 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.
> >
> > Not if you build using clang, because that doesn't support asm-goto and
> > thus falls back to a simple runtime variable, which is exactly what Kees
> > didn't want.
>
> Nah, Clang will get asm-goto soon. I'm not worried about that. Besides
> the "yay new target issue", I was concerned about Linus yelling about
> seeing the knob added he specifically said he didn't want. :P
>
This isn't really a knob. It can only get enabled when trace_printk()
is added, which requires a change to the kernel source code. It's just
an easy way on doing it yourself and recompiling.
No userspace can enable it, with the exception of adding a kernel
module that has a trace_printk() in it. A kernel command line option
wont even enable it.
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-04-04 16:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-03 19:41 [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used Steven Rostedt
2018-04-03 20:07 ` Kees Cook
2018-04-03 21:06 ` Steven Rostedt
2018-04-03 21:43 ` Tobin C. Harding
2018-04-03 22:59 ` Steven Rostedt
2018-04-04 7:49 ` Peter Zijlstra
2018-04-04 13:36 ` Steven Rostedt
2018-04-04 16:27 ` Kees Cook
2018-04-04 16:52 ` Steven Rostedt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.