On Thu, Jul 26, 2018 at 9:59 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Thu, Jul 26, 2018 at 9:37 AM Steven Rostedt rostedt@goodmis.org wrote:
On Thu, 26 Jul 2018 09:32:07 -0700 Nick Desaulniers ndesaulniers@google.com wrote:
On Thu, Jul 26, 2018 at 8:22 AM Steven Rostedt rostedt@goodmis.org wrote:
On Thu, 26 Jul 2018 08:14:08 -0700 Mark Salyzyn salyzyn@android.com wrote:
Thank you Steve, much appreciated feedback, I have asked the security developers to keep this in mind and come up with a correct fix.
The correct fix that meets your guidelines would _not_ be suitable for stable due to the invasiveness it sounds, only for the latest will such a rework make sense. As such, the fix proposed in this patch is the only one that meets the bar for stable patch simplicity, and merely(!) needs to state that if the fix is taken, perf and trace are broken.
Posting this patch publicly on the lists, that may never be applied, may be the limit of our responsibility for a fix to stable kernel releases, to be optionally applied by vendors concerned with this CVE criteria?
The patch breaks the code it touches. It makes it useless.
Doesn't that depend on kptr_restrict, or would it be broken if kptr_restrict was set to 0?
Is that what governs the output of kallsyms?
From my workstation:
$ cat /proc/kallsyms
prints a bunch of zero'd out addresses, while
$ sudo !!
prints out actual addresses. Looking at kernel/kallsyms.c, it seems that there's no use of %pK, but kallsyms_show_value() switches on kptr_restrict (and additional values):
/*
- We show kallsyms information even to normal users if we've enabled
- kernel profiling and are explicitly not paranoid (so kptr_restrict
- is clear, and sysctl_perf_event_paranoid isn't set).
- Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
- block even that).
*/ int kallsyms_show_value(void) { switch (kptr_restrict) { case 0: if (kallsyms_for_perf()) return 1; /* fallthrough */ case 1: if (has_capability_noaudit(current, CAP_SYSLOG)) return 1; /* fallthrough */ default: return 0; } }
What are folks thoughts on this: 1. create function show_symbols_for_perf() that replaces kallsyms_show_value(), maybe in linux/ftrace.c (since linux/ftrace.h is included in kernel/trace/trace_printk.c and kernel/kallsyms.c). 2. use new show_symbols_for_perf() in kernel/kallsyms.c and kernel/trace/trace_printk.c
Where the implementation of show_symbols_for_perf() is kallsyms_show_value() implementation-wise (just renamed since it's no longer kallsyms specific). Does that make sense, or should I just send a patch? Does it make sense to check whether kernel/trace/trace_printk.c#t_show() should print an address based on the same checks done in kallsyms_show_value()?