| Message ID | CA+55aFwiaQezaPi2nzmyuFZNgR3PFcsWjjBTHJYBesKrh-Q_pQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 7, 2017 at 4:59 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > For example, maybe /proc/kallsyms could just default to not showing > values to non-root users. > > Something like the attached TOTALLY UNTESTED patch. It's meant more as > an RFC, not for application, but it's also meant to show how we can > tailor the behavior for specific workflow issues. It seems to work, except I got the condition wrong for sysctl_perf_event_paranoid. It should if if (sysctl_perf_event_paranoid <= 1) return 1; rather than "<= 0", because '1' still means "allow kernel profiling" (and the default value is "2"). But I don't know if there is anything else than the profiling code that _really_ wants access to /proc/kallsyms in user space as a regular user. That said, that patch also fixes the /proc/kallsyms root check, in that now you can do: sudo head < /proc/kallsyms and it still shows all zeroes - because the file was *opened* as a normal user. That's how UNIX file access security works, and how it is fundamentally supposed to work (ie passing a file descriptor to a sui program doesn't magically make it gain privileges). Anyway, I'm obviously not going to commit that patch now, but I'd be happy to try it for the 4.15 merge window, to see if we can close /proc/kallsyms without people screaming.. Linus
> But I don't know if there is anything else than the profiling code > that _really_ wants access to /proc/kallsyms in user space as a > regular user. Am unsure about this, but kprobes? (/jprobes/kretprobes), and by extension, wrappers over this infra (like SystemTap)? I (hazily) recollect a script I once wrote (years back though) that collects kernel virtual addresses off of kallsyms for the purpose of passing them to a 'helper' kernel module that uses kprobes. I realize that 'modern' kprobes exposes APIs that just require the symbolic name & that they're anyway at kernel privilege... but the point is, a usermode script was picking up and passing the kernel addresses. Also, what about kernel addresses exposed via System.map? Oh, just checked, it's root rw only.. pl ignore. > That said, that patch also fixes the /proc/kallsyms root check, in > that now you can do: > > sudo head < /proc/kallsyms > > and it still shows all zeroes - because the file was *opened* as a > normal user. That's how UNIX file access security works, and how it is > fundamentally supposed to work (ie passing a file descriptor to a sui > program doesn't magically make it gain privileges). Indeed.
On Thu, Nov 9, 2017 at 10:13 AM, Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> wrote: >> But I don't know if there is anything else than the profiling code >> that _really_ wants access to /proc/kallsyms in user space as a >> regular user. > Front-ends to ftrace, like trace-cmd? [from the trace-cmd git repo (Steve Rostedt, pl stand up, pl stand up :-) Documentation/trace-cmd-restore.1.txt : ... *-k* kallsyms:: Used with *-c*, it overrides where to read the kallsyms file from. By default, /proc/kallsyms is used. *-k* will override the file to read the kallsyms from. This can be useful if the trace.dat file to create is from another machine. Just copy the /proc/kallsyms file locally, and use *-k* to point to that file. ... ] > Am unsure about this, but kprobes? (/jprobes/kretprobes), and by > extension, wrappers over this infra (like SystemTap)? > I (hazily) recollect a script I once wrote (years back though) that > collects kernel virtual addresses off of kallsyms for the purpose of > passing them to a 'helper' kernel module that uses kprobes. I realize > that 'modern' kprobes exposes APIs that just require the symbolic name > & that they're anyway at kernel privilege... but the point is, a > usermode script was picking up and passing the kernel addresses. > > Also, what about kernel addresses exposed via System.map? > Oh, just checked, it's root rw only.. pl ignore.
On Thu, 9 Nov 2017 10:24:32 +0530 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> wrote: > On Thu, Nov 9, 2017 at 10:13 AM, Kaiwan N Billimoria > <kaiwan.billimoria@gmail.com> wrote: > >> But I don't know if there is anything else than the profiling code > >> that _really_ wants access to /proc/kallsyms in user space as a > >> regular user. > > > > Front-ends to ftrace, like trace-cmd? > [from the trace-cmd git repo (Steve Rostedt, pl stand up, pl stand up :-) > Documentation/trace-cmd-restore.1.txt : Yes, profiling and tracing are similar. And you need to be root to run the recording anyway. Thus, as long as root user can read kallsyms, trace-cmd should be fine. As trace-cmd requires root access to do any ftrace tracing. -- Steve
> > Yes, profiling and tracing are similar. And you need to be root to run > the recording anyway. Thus, as long as root user can read kallsyms, > trace-cmd should be fine. As trace-cmd requires root access to do any > ftrace tracing. > > -- Steve Got it, thanks..
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 127e7cfafa55..5b1299c1e4b0 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -480,6 +480,7 @@ struct kallsym_iter { char name[KSYM_NAME_LEN]; char module_name[MODULE_NAME_LEN]; int exported; + int show_value; }; static int get_ksymbol_mod(struct kallsym_iter *iter) @@ -580,14 +581,23 @@ static void s_stop(struct seq_file *m, void *p) { } +#ifndef CONFIG_64BIT +# define KALLSYM_FMT "%08lx" +#else +# define KALLSYM_FMT "%016lx" +#endif + static int s_show(struct seq_file *m, void *p) { + unsigned long value; struct kallsym_iter *iter = m->private; /* Some debugging symbols have no name. Ignore them. */ if (!iter->name[0]) return 0; + value = iter->show_value ? iter->value : 0; + if (iter->module_name[0]) { char type; @@ -597,10 +607,10 @@ static int s_show(struct seq_file *m, void *p) */ type = iter->exported ? toupper(iter->type) : tolower(iter->type); - seq_printf(m, "%pK %c %s\t[%s]\n", (void *)iter->value, + seq_printf(m, KALLSYM_FMT " %c %s\t[%s]\n", value, type, iter->name, iter->module_name); } else - seq_printf(m, "%pK %c %s\n", (void *)iter->value, + seq_printf(m, KALLSYM_FMT " %c %s\n", value, iter->type, iter->name); return 0; } @@ -612,6 +622,40 @@ static const struct seq_operations kallsyms_op = { .show = s_show }; +static inline int kallsyms_for_perf(void) +{ +#ifdef CONFIG_PERF_EVENTS + extern int sysctl_perf_event_paranoid; + if (sysctl_perf_event_paranoid <= 0) + return 1; +#endif + return 0; +} + +/* + * 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). + */ +static 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; + } +} + static int kallsyms_open(struct inode *inode, struct file *file) { /* @@ -625,6 +669,7 @@ static int kallsyms_open(struct inode *inode, struct file *file) return -ENOMEM; reset_iter(iter, 0); + iter->show_value = kallsyms_show_value(); return 0; }