| Message ID | 20191011135458.7399da44@gandalf.local.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tracefs: Do not allocate and free proxy_ops for lockdown | expand |
On Fri, Oct 11, 2019 at 10:55 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > I bisected this down to the addition of the proxy_ops into tracefs for > lockdown. It appears that the allocation of the proxy_ops and then freeing > it in the destroy_inode callback, is causing havoc with the memory system. > Reading the documentation about destroy_inode, I'm not sure that this is the > proper way to handle allocating and then freeing the fops of the inode. What is happening is that *because* it has a "destroy_inode()" callback, it now expects destroy_inode to not just free free the proxy ops, but to also schedule the inode itself for freeing. Which that tracefs)destroy_inode() doesn't do. So the inode never gets free'd at all - and you eventually run out of memory due to the inode leak. The trivial fix would be to instead use the "free_inode()" callback (which is called after the required RCU grace period) and then free the proxy op there _and_ call free_inode_nonrcu() too. But I think your patch to not need a proxy op allocation at all is probably better. That said, I think the _best_ option would be to just getting rid of the proxy fops _entirely_, and just make the (very few) tracefs_create_file() cases do that LOCKDOWN_TRACEFS in their open in the first place. The proxy_fops was a hacky attempt to make the patch smaller. Your "just wrap all ops" thing now made the patch bigger than just doing the lockdown thing in all the callers. In fact, many of them use tracing_open_generic(). And others - like subsystem_open() already call tracing_open_generic() as part of their open. So from a quick glance, just making tracing_open_generic() do the lockdown testing will take care of most cases. Add a few other cases to fill up the whole set, and your'e done. Willing to do that instead? Linus
On Fri, 11 Oct 2019 11:20:30 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Willing to do that instead?
Honestly, what you described was my preferred solution ;-)
I just didn't want to upset the lockdown crowd if a new tracefs file
was opened without doing this.
Once locked down is set, can it ever be undone without rebooting? If
not, a lockdown call could also trigger setting tracing_disabled to 1.
Which is much stronger, as that was the code we added to kill tracing
if anything abnormal was detected (and it does a hard shutdown of all
the tracing utilities).
It's set to one on bootup and cleared, after tracing is initialized.
But it is never cleared again. If lockdown can be enabled at bootup, we
could simply not clear it, and we can have something to allow lockdown
to set it as well.
Currently, the only places tracing_disabled gets set is in the self
tests and if the ring buffer gets corrupted.
-- Steve
On Fri, Oct 11, 2019 at 11:36 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 11 Oct 2019 11:20:30 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > Willing to do that instead? > > Honestly, what you described was my preferred solution ;-) > > I just didn't want to upset the lockdown crowd if a new tracefs file > was opened without doing this. Well, since they introduced a bug in your code that killed your machine with the patch _they_ did, I don't think they get to complain when you fix it the way you (and me) want to... Fair is fair. Linus
On Fri, 2019-10-11 at 14:36 -0400, Steven Rostedt wrote: > On Fri, 11 Oct 2019 11:20:30 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > Willing to do that instead? > > Honestly, what you described was my preferred solution ;-) > > I just didn't want to upset the lockdown crowd if a new tracefs file > was opened without doing this. > > Once locked down is set, can it ever be undone without rebooting? [...] Earlier versions of the lockdown patch set added a magic SysRq command to turn it off. That's not currently present upstream but there may be plans to add it. Ben.
On Fri, 11 Oct 2019 11:20:30 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > So from a quick glance, just making tracing_open_generic() do the > lockdown testing will take care of most cases. Add a few other cases > to fill up the whole set, and your'e done. > > Willing to do that instead? OK, so I tried this approach, and there's a bit more than just a "few" extra cases that use tracing_open_generic(). Below is the full patch. Here's the diffstat: fs/tracefs/inode.c | 42 -------------------- kernel/trace/ftrace.c | 29 +++++++++++++- kernel/trace/trace.c | 73 ++++++++++++++++++++++++++++++++++-- kernel/trace/trace_dynevent.c | 5 ++ kernel/trace/trace_events.c | 10 ++++ kernel/trace/trace_events_hist.c | 11 +++++ kernel/trace/trace_events_trigger.c | 8 +++ kernel/trace/trace_kprobe.c | 11 +++++ kernel/trace/trace_printk.c | 7 +++ kernel/trace/trace_stack.c | 8 +++ kernel/trace/trace_stat.c | 6 ++ kernel/trace/trace_uprobe.c | 11 +++++ 12 files changed, 173 insertions(+), 48 deletions(-) Compared to the patch with the full wrapper: fs/tracefs/inode.c | 153 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 135 insertions(+), 18 deletions(-) I'm thinking that the full wrapper may not be as bad. But then again, I could probably clean up some of this code and have a single check for both the lockdown and the "tracing_disabled" check. -- Steve diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 9fc14e38927f..eeeae0475da9 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -20,7 +20,6 @@ #include <linux/parser.h> #include <linux/magic.h> #include <linux/slab.h> -#include <linux/security.h> #define TRACEFS_DEFAULT_MODE 0700 @@ -28,25 +27,6 @@ static struct vfsmount *tracefs_mount; static int tracefs_mount_count; static bool tracefs_registered; -static int default_open_file(struct inode *inode, struct file *filp) -{ - struct dentry *dentry = filp->f_path.dentry; - struct file_operations *real_fops; - int ret; - - if (!dentry) - return -EINVAL; - - ret = security_locked_down(LOCKDOWN_TRACEFS); - if (ret) - return ret; - - real_fops = dentry->d_fsdata; - if (!real_fops->open) - return 0; - return real_fops->open(inode, filp); -} - static ssize_t default_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -241,12 +221,6 @@ static int tracefs_apply_options(struct super_block *sb) return 0; } -static void tracefs_destroy_inode(struct inode *inode) -{ - if (S_ISREG(inode->i_mode)) - kfree(inode->i_fop); -} - static int tracefs_remount(struct super_block *sb, int *flags, char *data) { int err; @@ -283,7 +257,6 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root) static const struct super_operations tracefs_super_operations = { .statfs = simple_statfs, .remount_fs = tracefs_remount, - .destroy_inode = tracefs_destroy_inode, .show_options = tracefs_show_options, }; @@ -414,7 +387,6 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fops) { - struct file_operations *proxy_fops; struct dentry *dentry; struct inode *inode; @@ -430,20 +402,8 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, if (unlikely(!inode)) return failed_creating(dentry); - proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL); - if (unlikely(!proxy_fops)) { - iput(inode); - return failed_creating(dentry); - } - - if (!fops) - fops = &tracefs_file_operations; - - dentry->d_fsdata = (void *)fops; - memcpy(proxy_fops, fops, sizeof(*proxy_fops)); - proxy_fops->open = default_open_file; inode->i_mode = mode; - inode->i_fop = proxy_fops; + inode->i_fop = fops ? fops : &tracefs_file_operations; inode->i_private = data; d_instantiate(dentry, inode); fsnotify_create(dentry->d_parent->d_inode, dentry); diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 62a50bf399d6..326253b4de93 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -18,6 +18,7 @@ #include <linux/clocksource.h> #include <linux/sched/task.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/seq_file.h> #include <linux/tracefs.h> #include <linux/hardirq.h> @@ -3486,6 +3487,11 @@ static int ftrace_avail_open(struct inode *inode, struct file *file) { struct ftrace_iterator *iter; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (unlikely(ftrace_disabled)) return -ENODEV; @@ -3504,6 +3510,11 @@ static int ftrace_enabled_open(struct inode *inode, struct file *file) { struct ftrace_iterator *iter; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter)); if (!iter) @@ -3540,7 +3551,11 @@ ftrace_regex_open(struct ftrace_ops *ops, int flag, struct ftrace_hash *hash; struct list_head *mod_head; struct trace_array *tr = ops->private; - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; ftrace_ops_init(ops); @@ -3618,6 +3633,7 @@ ftrace_filter_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, inode, file); @@ -3628,6 +3644,7 @@ ftrace_notrace_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_NOTRACE, inode, file); } @@ -5194,9 +5211,13 @@ static int __ftrace_graph_open(struct inode *inode, struct file *file, struct ftrace_graph_data *fgd) { - int ret = 0; + int ret; struct ftrace_hash *new_hash = NULL; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (file->f_mode & FMODE_WRITE) { const int size_bits = FTRACE_HASH_DEFAULT_BITS; @@ -6537,6 +6558,10 @@ ftrace_pid_open(struct inode *inode, struct file *file) struct seq_file *m; int ret = 0; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (trace_array_get(tr) < 0) return -ENODEV; diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 252f79c435f8..4be1be27d064 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -17,6 +17,7 @@ #include <linux/stacktrace.h> #include <linux/writeback.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/seq_file.h> #include <linux/notifier.h> #include <linux/irqflags.h> @@ -4140,6 +4141,12 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot) int tracing_open_generic(struct inode *inode, struct file *filp) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -4159,6 +4166,11 @@ bool tracing_is_disabled(void) static int tracing_open_generic_tr(struct inode *inode, struct file *filp) { struct trace_array *tr = inode->i_private; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (tracing_disabled) return -ENODEV; @@ -4233,7 +4245,11 @@ static int tracing_open(struct inode *inode, struct file *file) { struct trace_array *tr = inode->i_private; struct trace_iterator *iter; - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (trace_array_get(tr) < 0) return -ENODEV; @@ -4352,6 +4368,10 @@ static int show_traces_open(struct inode *inode, struct file *file) struct seq_file *m; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -4697,6 +4717,10 @@ static int tracing_trace_options_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -5038,6 +5062,12 @@ static const struct seq_operations tracing_saved_tgids_seq_ops = { static int tracing_saved_tgids_open(struct inode *inode, struct file *filp) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -5115,6 +5145,12 @@ static const struct seq_operations tracing_saved_cmdlines_seq_ops = { static int tracing_saved_cmdlines_open(struct inode *inode, struct file *filp) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -5280,6 +5316,12 @@ static const struct seq_operations tracing_eval_map_seq_ops = { static int tracing_eval_map_open(struct inode *inode, struct file *filp) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -5804,7 +5846,11 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) { struct trace_array *tr = inode->i_private; struct trace_iterator *iter; - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (tracing_disabled) return -ENODEV; @@ -6547,6 +6593,10 @@ static int tracing_clock_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -6581,6 +6631,10 @@ static int tracing_time_stamp_mode_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; @@ -6638,7 +6692,11 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; struct trace_iterator *iter; struct seq_file *m; - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (trace_array_get(tr) < 0) return -ENODEV; @@ -6786,6 +6844,7 @@ static int snapshot_raw_open(struct inode *inode, struct file *filp) struct ftrace_buffer_info *info; int ret; + /* The following checks for tracefs lockdown */ ret = tracing_buffers_open(inode, filp); if (ret < 0) return ret; @@ -7105,6 +7164,10 @@ static int tracing_err_log_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret = 0; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (trace_array_get(tr) < 0) return -ENODEV; @@ -7157,6 +7220,10 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) struct ftrace_buffer_info *info; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (tracing_disabled) return -ENODEV; diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index a41fed46c285..7a731416bbdd 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -5,6 +5,7 @@ * Copyright (C) 2018 Masami Hiramatsu <mhiramat@kernel.org> */ +#include <linux/security.h> #include <linux/debugfs.h> #include <linux/kernel.h> #include <linux/list.h> @@ -174,6 +175,10 @@ static int dyn_event_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(NULL); if (ret < 0) diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index b89cdfe20bc1..cc02257fd6df 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -12,6 +12,7 @@ #define pr_fmt(fmt) fmt #include <linux/workqueue.h> +#include <linux/security.h> #include <linux/spinlock.h> #include <linux/kthread.h> #include <linux/tracefs.h> @@ -1294,6 +1295,10 @@ static int trace_format_open(struct inode *inode, struct file *file) struct seq_file *m; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = seq_open(file, &trace_format_seq_ops); if (ret < 0) return ret; @@ -1771,6 +1776,10 @@ ftrace_event_open(struct inode *inode, struct file *file, struct seq_file *m; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = seq_open(file, seq_ops); if (ret < 0) return ret; @@ -1795,6 +1804,7 @@ ftrace_event_avail_open(struct inode *inode, struct file *file) { const struct seq_operations *seq_ops = &show_event_seq_ops; + /* Checks for tracefs lockdown */ return ftrace_event_open(inode, file, seq_ops); } diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 9468bd8d44a2..71dfe6889d62 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -7,6 +7,7 @@ #include <linux/module.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/mutex.h> #include <linux/slab.h> #include <linux/stacktrace.h> @@ -1448,6 +1449,10 @@ static int synth_events_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&synth_event_ops); if (ret < 0) @@ -5515,6 +5520,12 @@ static int hist_show(struct seq_file *m, void *v) static int event_hist_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return single_open(file, hist_show, file); } diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c index 2a2912cb4533..2cd53ca21b51 100644 --- a/kernel/trace/trace_events_trigger.c +++ b/kernel/trace/trace_events_trigger.c @@ -5,6 +5,7 @@ * Copyright (C) 2013 Tom Zanussi <tom.zanussi@linux.intel.com> */ +#include <linux/security.h> #include <linux/module.h> #include <linux/ctype.h> #include <linux/mutex.h> @@ -173,7 +174,11 @@ static const struct seq_operations event_triggers_seq_ops = { static int event_trigger_regex_open(struct inode *inode, struct file *file) { - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; mutex_lock(&event_mutex); @@ -292,6 +297,7 @@ event_trigger_write(struct file *filp, const char __user *ubuf, static int event_trigger_open(struct inode *inode, struct file *filp) { + /* Checks for tracefs lockdown */ return event_trigger_regex_open(inode, filp); } diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 324ffbea3556..e4422746cb4c 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -7,6 +7,7 @@ */ #define pr_fmt(fmt) "trace_kprobe: " fmt +#include <linux/security.h> #include <linux/module.h> #include <linux/uaccess.h> #include <linux/rculist.h> @@ -936,6 +937,10 @@ static int probes_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&trace_kprobe_ops); if (ret < 0) @@ -988,6 +993,12 @@ static const struct seq_operations profile_seq_op = { static int profile_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &profile_seq_op); } diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c index c3fd849d4a8f..d4e31e969206 100644 --- a/kernel/trace/trace_printk.c +++ b/kernel/trace/trace_printk.c @@ -6,6 +6,7 @@ * */ #include <linux/seq_file.h> +#include <linux/security.h> #include <linux/uaccess.h> #include <linux/kernel.h> #include <linux/ftrace.h> @@ -348,6 +349,12 @@ static const struct seq_operations show_format_seq_ops = { static int ftrace_formats_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &show_format_seq_ops); } diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c index ec9a34a97129..4df9a209f7ca 100644 --- a/kernel/trace/trace_stack.c +++ b/kernel/trace/trace_stack.c @@ -5,6 +5,7 @@ */ #include <linux/sched/task_stack.h> #include <linux/stacktrace.h> +#include <linux/security.h> #include <linux/kallsyms.h> #include <linux/seq_file.h> #include <linux/spinlock.h> @@ -470,6 +471,12 @@ static const struct seq_operations stack_trace_seq_ops = { static int stack_trace_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &stack_trace_seq_ops); } @@ -487,6 +494,7 @@ stack_trace_filter_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_FILTER, inode, file); } diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c index 75bf1bcb4a8a..9ab0a1a7ad5e 100644 --- a/kernel/trace/trace_stat.c +++ b/kernel/trace/trace_stat.c @@ -9,7 +9,7 @@ * */ - +#include <linux/security.h> #include <linux/list.h> #include <linux/slab.h> #include <linux/rbtree.h> @@ -238,6 +238,10 @@ static int tracing_stat_open(struct inode *inode, struct file *file) struct seq_file *m; struct stat_session *session = inode->i_private; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = stat_seq_init(session); if (ret) return ret; diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index dd884341f5c5..352073d36585 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -7,6 +7,7 @@ */ #define pr_fmt(fmt) "trace_uprobe: " fmt +#include <linux/security.h> #include <linux/ctype.h> #include <linux/module.h> #include <linux/uaccess.h> @@ -769,6 +770,10 @@ static int probes_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&trace_uprobe_ops); if (ret) @@ -818,6 +823,12 @@ static const struct seq_operations profile_seq_op = { static int profile_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &profile_seq_op); }
On Fri, Oct 11, 2019 at 1:25 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > OK, so I tried this approach, and there's a bit more than just a "few" > extra cases that use tracing_open_generic(). Yeah, that's more than I would have expected. That said, you also did what I consider a somewhat over-done conversion. Just do static inline bool tracefs_lockdown_or_disabled(void) { return tracing_disabled || security_locked_down(LOCKDOWN_TRACEFS); } and ignore the pointless return value (we know it's EPERM, and we really don't care). And then several of those conversions just turn into oneliner - if (tracing_disabled) + if (tracefs_lockdown_or_disabled()) return -ENODEV; patches instead. I'm also not sure why you bothered with a lot of the files that don't currently even have that "tracing_disabled" logic at all. Yeah, they show pre-existing tracing info, but even if you locked down _after_ starting some tracing, that's probably what you want. You just don't want people to be able to add new trace events. For example, maybe you want to have lockdown after you've booted, but still show boot time traces? I dunno. I feel like you re-did the original patch, and the original patch was designed for "least line impact" rather than for anything else. I also suspect that if you *actually* do lockdown at early boot (which is presumably one common case), then all you need is to do --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -418,6 +418,9 @@ struct dentry *tracefs_create_file( struct dentry *dentry; struct inode *inode; + if (security_locked_down(LOCKDOWN_TRACEFS)) + return NULL; + if (!(mode & S_IFMT)) mode |= S_IFREG; BUG_ON(!S_ISREG(mode)); and the "open-time check" is purely for "we did lockdown _after_ boot, but then you might well want to be able to read the existing trace information that you did before you locked down. Again - I think trying to emulate exactly what that broken lockdown patch did is not really what you should aim for. That patch was not only buggy, it really wasn't about what people really necessarily _want_, as about "we don't want to deal with tracing, so here's a minimal patch that doesn't even work". Linus
On Fri, 11 Oct 2019 16:25:18 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > > Here's the diffstat: > > fs/tracefs/inode.c | 42 -------------------- > kernel/trace/ftrace.c | 29 +++++++++++++- > kernel/trace/trace.c | 73 ++++++++++++++++++++++++++++++++++-- > kernel/trace/trace_dynevent.c | 5 ++ > kernel/trace/trace_events.c | 10 ++++ > kernel/trace/trace_events_hist.c | 11 +++++ > kernel/trace/trace_events_trigger.c | 8 +++ > kernel/trace/trace_kprobe.c | 11 +++++ > kernel/trace/trace_printk.c | 7 +++ > kernel/trace/trace_stack.c | 8 +++ > kernel/trace/trace_stat.c | 6 ++ > kernel/trace/trace_uprobe.c | 11 +++++ > 12 files changed, 173 insertions(+), 48 deletions(-) > > Compared to the patch with the full wrapper: > > fs/tracefs/inode.c | 153 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 135 insertions(+), 18 deletions(-) > Consolidating some of the functions did a little better on the deletion front: fs/tracefs/inode.c | 42 ------------ kernel/trace/ftrace.c | 30 +++++++- kernel/trace/trace.c | 122 +++++++++++++++++++++--------------- kernel/trace/trace.h | 1 kernel/trace/trace_dynevent.c | 5 + kernel/trace/trace_events.c | 41 ++++-------- kernel/trace/trace_events_hist.c | 11 +++ kernel/trace/trace_events_trigger.c | 8 ++ kernel/trace/trace_kprobe.c | 11 +++ kernel/trace/trace_printk.c | 7 ++ kernel/trace/trace_stack.c | 8 ++ kernel/trace/trace_stat.c | 6 + kernel/trace/trace_uprobe.c | 11 +++ 13 files changed, 181 insertions(+), 122 deletions(-) I guess I can keep it this way. Thoughts? -- Steve diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 9fc14e38927f..eeeae0475da9 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -20,7 +20,6 @@ #include <linux/parser.h> #include <linux/magic.h> #include <linux/slab.h> -#include <linux/security.h> #define TRACEFS_DEFAULT_MODE 0700 @@ -28,25 +27,6 @@ static struct vfsmount *tracefs_mount; static int tracefs_mount_count; static bool tracefs_registered; -static int default_open_file(struct inode *inode, struct file *filp) -{ - struct dentry *dentry = filp->f_path.dentry; - struct file_operations *real_fops; - int ret; - - if (!dentry) - return -EINVAL; - - ret = security_locked_down(LOCKDOWN_TRACEFS); - if (ret) - return ret; - - real_fops = dentry->d_fsdata; - if (!real_fops->open) - return 0; - return real_fops->open(inode, filp); -} - static ssize_t default_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -241,12 +221,6 @@ static int tracefs_apply_options(struct super_block *sb) return 0; } -static void tracefs_destroy_inode(struct inode *inode) -{ - if (S_ISREG(inode->i_mode)) - kfree(inode->i_fop); -} - static int tracefs_remount(struct super_block *sb, int *flags, char *data) { int err; @@ -283,7 +257,6 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root) static const struct super_operations tracefs_super_operations = { .statfs = simple_statfs, .remount_fs = tracefs_remount, - .destroy_inode = tracefs_destroy_inode, .show_options = tracefs_show_options, }; @@ -414,7 +387,6 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fops) { - struct file_operations *proxy_fops; struct dentry *dentry; struct inode *inode; @@ -430,20 +402,8 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, if (unlikely(!inode)) return failed_creating(dentry); - proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL); - if (unlikely(!proxy_fops)) { - iput(inode); - return failed_creating(dentry); - } - - if (!fops) - fops = &tracefs_file_operations; - - dentry->d_fsdata = (void *)fops; - memcpy(proxy_fops, fops, sizeof(*proxy_fops)); - proxy_fops->open = default_open_file; inode->i_mode = mode; - inode->i_fop = proxy_fops; + inode->i_fop = fops ? fops : &tracefs_file_operations; inode->i_private = data; d_instantiate(dentry, inode); fsnotify_create(dentry->d_parent->d_inode, dentry); diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 62a50bf399d6..7bcbfef9ab56 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -18,6 +18,7 @@ #include <linux/clocksource.h> #include <linux/sched/task.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/seq_file.h> #include <linux/tracefs.h> #include <linux/hardirq.h> @@ -3486,6 +3487,11 @@ static int ftrace_avail_open(struct inode *inode, struct file *file) { struct ftrace_iterator *iter; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; if (unlikely(ftrace_disabled)) return -ENODEV; @@ -3504,6 +3510,11 @@ static int ftrace_enabled_open(struct inode *inode, struct file *file) { struct ftrace_iterator *iter; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter)); if (!iter) @@ -3540,7 +3551,11 @@ ftrace_regex_open(struct ftrace_ops *ops, int flag, struct ftrace_hash *hash; struct list_head *mod_head; struct trace_array *tr = ops->private; - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; ftrace_ops_init(ops); @@ -3618,6 +3633,7 @@ ftrace_filter_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, inode, file); @@ -3628,6 +3644,7 @@ ftrace_notrace_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_NOTRACE, inode, file); } @@ -5194,9 +5211,13 @@ static int __ftrace_graph_open(struct inode *inode, struct file *file, struct ftrace_graph_data *fgd) { - int ret = 0; + int ret; struct ftrace_hash *new_hash = NULL; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if (file->f_mode & FMODE_WRITE) { const int size_bits = FTRACE_HASH_DEFAULT_BITS; @@ -6537,8 +6558,9 @@ ftrace_pid_open(struct inode *inode, struct file *file) struct seq_file *m; int ret = 0; - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 252f79c435f8..d6e467281bbc 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -17,6 +17,7 @@ #include <linux/stacktrace.h> #include <linux/writeback.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/seq_file.h> #include <linux/notifier.h> #include <linux/irqflags.h> @@ -304,6 +305,23 @@ void trace_array_put(struct trace_array *this_tr) mutex_unlock(&trace_types_lock); } +int tracing_check_open_get_tr(struct trace_array *tr) +{ + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + + if (tracing_disabled) + return -ENODEV; + + if (tr && trace_array_get(tr) < 0) + return -ENODEV; + + return 0; +} + int call_filter_check_discard(struct trace_event_call *call, void *rec, struct ring_buffer *buffer, struct ring_buffer_event *event) @@ -4140,8 +4158,11 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot) int tracing_open_generic(struct inode *inode, struct file *filp) { - if (tracing_disabled) - return -ENODEV; + int ret; + + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; filp->private_data = inode->i_private; return 0; @@ -4159,12 +4180,11 @@ bool tracing_is_disabled(void) static int tracing_open_generic_tr(struct inode *inode, struct file *filp) { struct trace_array *tr = inode->i_private; + int ret; - if (tracing_disabled) - return -ENODEV; - - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; filp->private_data = inode->i_private; @@ -4233,10 +4253,11 @@ static int tracing_open(struct inode *inode, struct file *file) { struct trace_array *tr = inode->i_private; struct trace_iterator *iter; - int ret = 0; + int ret; - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; /* If this file was open for write, then erase contents */ if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { @@ -4352,8 +4373,9 @@ static int show_traces_open(struct inode *inode, struct file *file) struct seq_file *m; int ret; - if (tracing_disabled) - return -ENODEV; + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; ret = seq_open(file, &show_traces_seq_ops); if (ret) @@ -4697,11 +4719,9 @@ static int tracing_trace_options_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; - if (tracing_disabled) - return -ENODEV; - - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; ret = single_open(file, tracing_trace_options_show, inode->i_private); if (ret < 0) @@ -5038,8 +5058,11 @@ static const struct seq_operations tracing_saved_tgids_seq_ops = { static int tracing_saved_tgids_open(struct inode *inode, struct file *filp) { - if (tracing_disabled) - return -ENODEV; + int ret; + + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; return seq_open(filp, &tracing_saved_tgids_seq_ops); } @@ -5115,8 +5138,11 @@ static const struct seq_operations tracing_saved_cmdlines_seq_ops = { static int tracing_saved_cmdlines_open(struct inode *inode, struct file *filp) { - if (tracing_disabled) - return -ENODEV; + int ret; + + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; return seq_open(filp, &tracing_saved_cmdlines_seq_ops); } @@ -5280,8 +5306,11 @@ static const struct seq_operations tracing_eval_map_seq_ops = { static int tracing_eval_map_open(struct inode *inode, struct file *filp) { - if (tracing_disabled) - return -ENODEV; + int ret; + + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; return seq_open(filp, &tracing_eval_map_seq_ops); } @@ -5804,13 +5833,11 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) { struct trace_array *tr = inode->i_private; struct trace_iterator *iter; - int ret = 0; - - if (tracing_disabled) - return -ENODEV; + int ret; - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; mutex_lock(&trace_types_lock); @@ -6547,11 +6574,9 @@ static int tracing_clock_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; - if (tracing_disabled) - return -ENODEV; - - if (trace_array_get(tr)) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; ret = single_open(file, tracing_clock_show, inode->i_private); if (ret < 0) @@ -6581,11 +6606,9 @@ static int tracing_time_stamp_mode_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret; - if (tracing_disabled) - return -ENODEV; - - if (trace_array_get(tr)) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; ret = single_open(file, tracing_time_stamp_mode_show, inode->i_private); if (ret < 0) @@ -6638,10 +6661,11 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; struct trace_iterator *iter; struct seq_file *m; - int ret = 0; + int ret; - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; if (file->f_mode & FMODE_READ) { iter = __tracing_open(inode, file, true); @@ -6786,6 +6810,7 @@ static int snapshot_raw_open(struct inode *inode, struct file *filp) struct ftrace_buffer_info *info; int ret; + /* The following checks for tracefs lockdown */ ret = tracing_buffers_open(inode, filp); if (ret < 0) return ret; @@ -7105,8 +7130,9 @@ static int tracing_err_log_open(struct inode *inode, struct file *file) struct trace_array *tr = inode->i_private; int ret = 0; - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; /* If this file was opened for write, then erase contents */ if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) @@ -7157,11 +7183,9 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) struct ftrace_buffer_info *info; int ret; - if (tracing_disabled) - return -ENODEV; - - if (trace_array_get(tr) < 0) - return -ENODEV; + ret = tracing_check_open_get_tr(tr); + if (ret) + return ret; info = kzalloc(sizeof(*info), GFP_KERNEL); if (!info) { diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index f801d154ff6a..fc41971306c1 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -338,6 +338,7 @@ extern struct mutex trace_types_lock; extern int trace_array_get(struct trace_array *tr); extern void trace_array_put(struct trace_array *tr); +extern int tracing_check_open_get_tr(struct trace_array *tr); extern int tracing_set_time_stamp_abs(struct trace_array *tr, bool abs); extern int tracing_set_clock(struct trace_array *tr, const char *clockstr); diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index a41fed46c285..74df50375fd9 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -5,6 +5,7 @@ * Copyright (C) 2018 Masami Hiramatsu <mhiramat@kernel.org> */ +#include <linux/security.h> #include <linux/debugfs.h> #include <linux/kernel.h> #include <linux/list.h> @@ -174,6 +175,10 @@ static int dyn_event_open(struct inode *inode, struct file *file) { int ret; + ret = tracing_check_open_get_tr(NULL); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(NULL); if (ret < 0) diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index b89cdfe20bc1..24a642489437 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -12,6 +12,7 @@ #define pr_fmt(fmt) fmt #include <linux/workqueue.h> +#include <linux/security.h> #include <linux/spinlock.h> #include <linux/kthread.h> #include <linux/tracefs.h> @@ -1294,6 +1295,10 @@ static int trace_format_open(struct inode *inode, struct file *file) struct seq_file *m; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = seq_open(file, &trace_format_seq_ops); if (ret < 0) return ret; @@ -1391,9 +1396,6 @@ static int subsystem_open(struct inode *inode, struct file *filp) struct trace_array *tr; int ret; - if (tracing_is_disabled()) - return -ENODEV; - /* Make sure the system still exists */ mutex_lock(&event_mutex); mutex_lock(&trace_types_lock); @@ -1420,16 +1422,9 @@ static int subsystem_open(struct inode *inode, struct file *filp) WARN_ON(!dir); /* Still need to increment the ref count of the system */ - if (trace_array_get(tr) < 0) { - put_system(dir); - return -ENODEV; - } - - ret = tracing_open_generic(inode, filp); - if (ret < 0) { - trace_array_put(tr); + ret = tracing_open_generic_tr(inode, filp); + if (ret < 0) put_system(dir); - } return ret; } @@ -1440,28 +1435,17 @@ static int system_tr_open(struct inode *inode, struct file *filp) struct trace_array *tr = inode->i_private; int ret; - if (tracing_is_disabled()) - return -ENODEV; - - if (trace_array_get(tr) < 0) - return -ENODEV; - /* Make a temporary dir that has no system but points to tr */ dir = kzalloc(sizeof(*dir), GFP_KERNEL); - if (!dir) { - trace_array_put(tr); + if (!dir) return -ENOMEM; - } - - dir->tr = tr; - ret = tracing_open_generic(inode, filp); + ret = tracing_open_generic_tr(inode, filp); if (ret < 0) { - trace_array_put(tr); kfree(dir); return ret; } - + dir->tr = tr; filp->private_data = dir; return 0; @@ -1771,6 +1755,10 @@ ftrace_event_open(struct inode *inode, struct file *file, struct seq_file *m; int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = seq_open(file, seq_ops); if (ret < 0) return ret; @@ -1795,6 +1783,7 @@ ftrace_event_avail_open(struct inode *inode, struct file *file) { const struct seq_operations *seq_ops = &show_event_seq_ops; + /* Checks for tracefs lockdown */ return ftrace_event_open(inode, file, seq_ops); } diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 9468bd8d44a2..71dfe6889d62 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -7,6 +7,7 @@ #include <linux/module.h> #include <linux/kallsyms.h> +#include <linux/security.h> #include <linux/mutex.h> #include <linux/slab.h> #include <linux/stacktrace.h> @@ -1448,6 +1449,10 @@ static int synth_events_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&synth_event_ops); if (ret < 0) @@ -5515,6 +5520,12 @@ static int hist_show(struct seq_file *m, void *v) static int event_hist_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return single_open(file, hist_show, file); } diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c index 2a2912cb4533..2cd53ca21b51 100644 --- a/kernel/trace/trace_events_trigger.c +++ b/kernel/trace/trace_events_trigger.c @@ -5,6 +5,7 @@ * Copyright (C) 2013 Tom Zanussi <tom.zanussi@linux.intel.com> */ +#include <linux/security.h> #include <linux/module.h> #include <linux/ctype.h> #include <linux/mutex.h> @@ -173,7 +174,11 @@ static const struct seq_operations event_triggers_seq_ops = { static int event_trigger_regex_open(struct inode *inode, struct file *file) { - int ret = 0; + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; mutex_lock(&event_mutex); @@ -292,6 +297,7 @@ event_trigger_write(struct file *filp, const char __user *ubuf, static int event_trigger_open(struct inode *inode, struct file *filp) { + /* Checks for tracefs lockdown */ return event_trigger_regex_open(inode, filp); } diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 324ffbea3556..e4422746cb4c 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -7,6 +7,7 @@ */ #define pr_fmt(fmt) "trace_kprobe: " fmt +#include <linux/security.h> #include <linux/module.h> #include <linux/uaccess.h> #include <linux/rculist.h> @@ -936,6 +937,10 @@ static int probes_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&trace_kprobe_ops); if (ret < 0) @@ -988,6 +993,12 @@ static const struct seq_operations profile_seq_op = { static int profile_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &profile_seq_op); } diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c index c3fd849d4a8f..d4e31e969206 100644 --- a/kernel/trace/trace_printk.c +++ b/kernel/trace/trace_printk.c @@ -6,6 +6,7 @@ * */ #include <linux/seq_file.h> +#include <linux/security.h> #include <linux/uaccess.h> #include <linux/kernel.h> #include <linux/ftrace.h> @@ -348,6 +349,12 @@ static const struct seq_operations show_format_seq_ops = { static int ftrace_formats_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &show_format_seq_ops); } diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c index ec9a34a97129..4df9a209f7ca 100644 --- a/kernel/trace/trace_stack.c +++ b/kernel/trace/trace_stack.c @@ -5,6 +5,7 @@ */ #include <linux/sched/task_stack.h> #include <linux/stacktrace.h> +#include <linux/security.h> #include <linux/kallsyms.h> #include <linux/seq_file.h> #include <linux/spinlock.h> @@ -470,6 +471,12 @@ static const struct seq_operations stack_trace_seq_ops = { static int stack_trace_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &stack_trace_seq_ops); } @@ -487,6 +494,7 @@ stack_trace_filter_open(struct inode *inode, struct file *file) { struct ftrace_ops *ops = inode->i_private; + /* Checks for tracefs lockdown */ return ftrace_regex_open(ops, FTRACE_ITER_FILTER, inode, file); } diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c index 75bf1bcb4a8a..9ab0a1a7ad5e 100644 --- a/kernel/trace/trace_stat.c +++ b/kernel/trace/trace_stat.c @@ -9,7 +9,7 @@ * */ - +#include <linux/security.h> #include <linux/list.h> #include <linux/slab.h> #include <linux/rbtree.h> @@ -238,6 +238,10 @@ static int tracing_stat_open(struct inode *inode, struct file *file) struct seq_file *m; struct stat_session *session = inode->i_private; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + ret = stat_seq_init(session); if (ret) return ret; diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index dd884341f5c5..352073d36585 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -7,6 +7,7 @@ */ #define pr_fmt(fmt) "trace_uprobe: " fmt +#include <linux/security.h> #include <linux/ctype.h> #include <linux/module.h> #include <linux/uaccess.h> @@ -769,6 +770,10 @@ static int probes_open(struct inode *inode, struct file *file) { int ret; + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { ret = dyn_events_release_all(&trace_uprobe_ops); if (ret) @@ -818,6 +823,12 @@ static const struct seq_operations profile_seq_op = { static int profile_open(struct inode *inode, struct file *file) { + int ret; + + ret = security_locked_down(LOCKDOWN_TRACEFS); + if (ret) + return ret; + return seq_open(file, &profile_seq_op); }
On Fri, Oct 11, 2019 at 1:55 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > I guess I can keep it this way. Thoughts? That looks fine to me. I'm still not sure you actually need to do all this, but it doesn't look _wrong_. That said, I still do think that if things are locked down from the very get-go, tracefs_create_file() shouldn't even create the files. That's mostly an independent thing from the "what about if they exists and things got locked down afterwards", though. I do wonder about the whole "well, if you started tracing before locking things down, don't you want to see the end results"? Linus
On Fri, 11 Oct 2019 13:46:11 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Oct 11, 2019 at 1:25 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > OK, so I tried this approach, and there's a bit more than just a "few" > > extra cases that use tracing_open_generic(). > > Yeah, that's more than I would have expected. > > That said, you also did what I consider a somewhat over-done conversion. > > Just do > > static inline bool tracefs_lockdown_or_disabled(void) > { return tracing_disabled || security_locked_down(LOCKDOWN_TRACEFS); } > > and ignore the pointless return value (we know it's EPERM, and we > really don't care). > > And then several of those conversions just turn into oneliner > > - if (tracing_disabled) > + if (tracefs_lockdown_or_disabled()) > return -ENODEV; OK, so this is similar to what I just sent. But instead I made it into a function that combines tracing_disabled and the locked_down, plus, it did the reference update for the trace_array if needed (which in doing this exercise, I think I found a couple of places that need the ref count update). > > patches instead. > > I'm also not sure why you bothered with a lot of the files that don't > currently even have that "tracing_disabled" logic at all. Yeah, they > show pre-existing tracing info, but even if you locked down _after_ > starting some tracing, that's probably what you want. You just don't > want people to be able to add new trace events. I guess just preventing new traces would be good enough (or anything that records data), as well as seeing that recorded data. Note, the tracing_disabled was added in case the ring buffer got corrupted, and we wanted to prevent the system from crashing if someone read the corrupted ring buffer. In the over 10 years of having that check, I don't recall a single instance of someone triggering it :-/ > > For example, maybe you want to have lockdown after you've booted, but > still show boot time traces? > > I dunno. I feel like you re-did the original patch, and the original > patch was designed for "least line impact" rather than for anything > else. > > I also suspect that if you *actually* do lockdown at early boot (which > is presumably one common case), then all you need is to do > > --- a/fs/tracefs/inode.c > +++ b/fs/tracefs/inode.c > @@ -418,6 +418,9 @@ struct dentry *tracefs_create_file( > struct dentry *dentry; > struct inode *inode; > > + if (security_locked_down(LOCKDOWN_TRACEFS)) > + return NULL; > + Sounds reasonable. > if (!(mode & S_IFMT)) > mode |= S_IFREG; > BUG_ON(!S_ISREG(mode)); > > and the "open-time check" is purely for "we did lockdown _after_ boot, > but then you might well want to be able to read the existing trace > information that you did before you locked down. > > Again - I think trying to emulate exactly what that broken lockdown > patch did is not really what you should aim for. > > That patch was not only buggy, it really wasn't about what people > really necessarily _want_, as about "we don't want to deal with > tracing, so here's a minimal patch that doesn't even work". OK. My new approach is to: 1) revert the tracefs lockdown patch 2) Add my tracing_check_open_get_tr() patch (and consolidate the calls) (fix up anything that should have this too) 3) Add the lockdown to the tracing_check_open_get_tr() routine, and be done with it. -- Steve
On Fri, 11 Oct 2019 14:00:50 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Oct 11, 2019 at 1:55 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > I guess I can keep it this way. Thoughts? > > That looks fine to me. I'm still not sure you actually need to do all > this, but it doesn't look _wrong_. Yep, I sent this before seeing your other email. > > That said, I still do think that if things are locked down from the > very get-go, tracefs_create_file() shouldn't even create the files. Agreed. I forgot to add in my last email: 4) Add the lockdown check to create_file() > > That's mostly an independent thing from the "what about if they exists > and things got locked down afterwards", though. Well, I'll be combining it with the tracing_disabled code, which was there originally to prevent corrupted buffers from causing harm by being read. > > I do wonder about the whole "well, if you started tracing before > locking things down, don't you want to see the end results"? > I don't think it hurts to disable it. Although, I don't think reading the trace event formats will be a issue. I'm not aware of any of them from giving too much info to the system. And if you really do care about that, do it at boot up, and with the create_file part, it wont have any files to read. -- Steve
* Steven Rostedt:
> Once locked down is set, can it ever be undone without rebooting?
I think this is the original intent with such patches, yes. But then
reality interferes and people add some escape hatch, so that it's
possible again to load arbitrary kernel modules. And for servers, you
can't have a meaningful physical presence check, so you end up with a
lot of complexity for something that offers absolutely zero gains in
security.
The other practical issue is that general-purpose Linux distributions
cannot prevent kernel downgrades, so even if there's a
cryptographically signed chain from the firmware to the kernel, you
can boot last year's kernel, use a root-to-ring-0 exploit to disable
its particular implementation of lockdown, and then kexec the real
kernel with lockdown disabled.
I'm sure that kernel lockdown has applications somewhere, but for
general-purpose distributions (who usually want to support third-party
kernel modules), it's an endless source of problems that wouldn't
exist without it.
On Fri, 11 Oct 2019 23:46:20 +0200 Florian Weimer <fw@deneb.enyo.de> wrote: > * Steven Rostedt: > > > Once locked down is set, can it ever be undone without rebooting? > > I think this is the original intent with such patches, yes. But then > reality interferes and people add some escape hatch, so that it's > possible again to load arbitrary kernel modules. And for servers, you > can't have a meaningful physical presence check, so you end up with a > lot of complexity for something that offers absolutely zero gains in > security. > > The other practical issue is that general-purpose Linux distributions > cannot prevent kernel downgrades, so even if there's a > cryptographically signed chain from the firmware to the kernel, you > can boot last year's kernel, use a root-to-ring-0 exploit to disable > its particular implementation of lockdown, and then kexec the real > kernel with lockdown disabled. > > I'm sure that kernel lockdown has applications somewhere, but for > general-purpose distributions (who usually want to support third-party > kernel modules), it's an endless source of problems that wouldn't > exist without it. I just decided to keep the two separate. The tracing_disable is permanent (unless you actually do something that writes into kernel memory to change the variable). When set, there's nothing to clear it. Thus, I decided not to couple that with lockdown, and let the lockdown folks do whatever they damn well please ;-) -- Steve
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 9fc14e38927f..d0e8e4a16812 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -20,6 +20,7 @@ #include <linux/parser.h> #include <linux/magic.h> #include <linux/slab.h> +#include <linux/poll.h> #include <linux/security.h> #define TRACEFS_DEFAULT_MODE 0700 @@ -28,7 +29,7 @@ static struct vfsmount *tracefs_mount; static int tracefs_mount_count; static bool tracefs_registered; -static int default_open_file(struct inode *inode, struct file *filp) +static int proxy_open(struct inode *inode, struct file *filp) { struct dentry *dentry = filp->f_path.dentry; struct file_operations *real_fops; @@ -47,6 +48,138 @@ static int default_open_file(struct inode *inode, struct file *filp) return real_fops->open(inode, filp); } +static ssize_t proxy_read(struct file *file, char __user *buf, + size_t count, loff_t *pos) +{ + struct dentry *dentry = file->f_path.dentry; + struct file_operations *real_fops; + + if (!dentry) + return -EINVAL; + + real_fops = dentry->d_fsdata; + + if (real_fops->read) + return real_fops->read(file, buf, count, pos); + else + return -EINVAL; +} + +static ssize_t proxy_write(struct file *file, const char __user *p, + size_t count, loff_t *pos) +{ + struct dentry *dentry = file->f_path.dentry; + struct file_operations *real_fops; + + if (!dentry) + return -EINVAL; + + real_fops = dentry->d_fsdata; + + if (real_fops->write) + return real_fops->write(file, p, count, pos); + else + return -EINVAL; +} + +static loff_t proxy_llseek(struct file *file, loff_t offset, int whence) +{ + struct dentry *dentry = file->f_path.dentry; + struct file_operations *real_fops; + loff_t (*fn)(struct file *, loff_t, int); + + if (!dentry) + return -EINVAL; + + real_fops = dentry->d_fsdata; + + fn = no_llseek; + if (file->f_mode & FMODE_LSEEK) { + if (real_fops->llseek) + fn = real_fops->llseek; + } + return fn(file, offset, whence); +} + +static int proxy_release(struct inode *inode, struct file *filp) +{ + struct dentry *dentry = filp->f_path.dentry; + struct file_operations *real_fops; + + if (!dentry) + return 0; + + real_fops = dentry->d_fsdata; + + if (real_fops->release) + return real_fops->release(inode, filp); + return 0; +} + +static __poll_t proxy_poll(struct file *file, struct poll_table_struct *pt) +{ + struct dentry *dentry = file->f_path.dentry; + struct file_operations *real_fops; + + if (!dentry) + return 0; + + real_fops = dentry->d_fsdata; + + if (unlikely(!real_fops->poll)) + return DEFAULT_POLLMASK; + return real_fops->poll(file, pt); +} + +static ssize_t proxy_splice_read(struct file *in, loff_t *ppos, + struct pipe_inode_info *pipe, size_t len, + unsigned int flags) +{ + struct dentry *dentry = in->f_path.dentry; + struct file_operations *real_fops; + ssize_t (*splice_read)(struct file *, loff_t *, + struct pipe_inode_info *, size_t, unsigned int); + + if (!dentry) + return 0; + + real_fops = dentry->d_fsdata; + + if (real_fops->splice_read) + splice_read = real_fops->splice_read; + else + splice_read = generic_file_splice_read; + + return splice_read(in, ppos, pipe, len, flags); +} + +static int proxy_mmap(struct file *file, struct vm_area_struct *vma) +{ + struct dentry *dentry = file->f_path.dentry; + struct file_operations *real_fops; + + if (!dentry) + return 0; + + real_fops = dentry->d_fsdata; + + if (!real_fops->mmap) + return -ENODEV; + + return real_fops->mmap(file, vma); +} + +static const struct file_operations proxy_fops = { + .open = proxy_open, + .read = proxy_read, + .write = proxy_write, + .llseek = proxy_llseek, + .release = proxy_release, + .poll = proxy_poll, + .splice_read = proxy_splice_read, + .mmap = proxy_mmap, +}; + static ssize_t default_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -241,12 +374,6 @@ static int tracefs_apply_options(struct super_block *sb) return 0; } -static void tracefs_destroy_inode(struct inode *inode) -{ - if (S_ISREG(inode->i_mode)) - kfree(inode->i_fop); -} - static int tracefs_remount(struct super_block *sb, int *flags, char *data) { int err; @@ -283,7 +410,6 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root) static const struct super_operations tracefs_super_operations = { .statfs = simple_statfs, .remount_fs = tracefs_remount, - .destroy_inode = tracefs_destroy_inode, .show_options = tracefs_show_options, }; @@ -414,7 +540,6 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fops) { - struct file_operations *proxy_fops; struct dentry *dentry; struct inode *inode; @@ -430,20 +555,12 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode, if (unlikely(!inode)) return failed_creating(dentry); - proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL); - if (unlikely(!proxy_fops)) { - iput(inode); - return failed_creating(dentry); - } - if (!fops) fops = &tracefs_file_operations; dentry->d_fsdata = (void *)fops; - memcpy(proxy_fops, fops, sizeof(*proxy_fops)); - proxy_fops->open = default_open_file; inode->i_mode = mode; - inode->i_fop = proxy_fops; + inode->i_fop = &proxy_fops; inode->i_private = data; d_instantiate(dentry, inode); fsnotify_create(dentry->d_parent->d_inode, dentry);