lkml.org 
[lkml]   [2018]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
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.

\
 
 \ /
  Last update: 2018-04-03 23:44    [W:0.082 / U:10.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site