Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt 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_d40g4mucssvrzzrfpujt74vc6pppb675hynx...@mail.gmail.com -Kees -- Kees Cook Pixel Security
Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Tue, 3 Apr 2018 13:07:58 -0700 Kees Cook wrote: > On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt 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_d40g4mucssvrzzrfpujt74vc6pppb675hynx...@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); }
Re: [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 wrote: > > > On Tue, Apr 3, 2018 at 12:41 PM, Steven Rostedt 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_d40g4mucssvrzzrfpujt74vc6pppb675hynx...@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.
Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Wed, 4 Apr 2018 07:43:49 +1000 "Tobin C. Harding" 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
Re: [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: > 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.
Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Wed, 4 Apr 2018 09:49:27 +0200 Peter Zijlstra 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
Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Wed, Apr 4, 2018 at 12:49 AM, Peter Zijlstra 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
Re: [RFC][PATCH] tracing, printk: Force no hashing when trace_printk() is used
On Wed, 4 Apr 2018 09:27:10 -0700 Kees Cook wrote: > On Wed, Apr 4, 2018 at 12:49 AM, Peter Zijlstra 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
