Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
On Wed, 4 Apr 2018 12:04:42 -0700 Linus Torvalds 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
Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
On Wed, Apr 4, 2018 at 10:13 AM, Steven Rostedt 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
Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
On Wed, 4 Apr 2018 11:34:55 +0900 Sergey Senozhatsky 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
Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
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
Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
On Tue, 3 Apr 2018 14:43:43 -0700 Kees Cook 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
Re: [PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
On Tue, Apr 3, 2018 at 2:31 PM, Steven Rostedt wrote: > From: Steven Rostedt (VMware) > > 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
[PATCH v2] tracing, printk: Force no hashing when trace_printk() is used
From: Steven Rostedt (VMware) 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.150b1...@gandalf.local.home Signed-off-by: Steven Rostedt (VMware) --- 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