* [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown @ 2019-10-11 17:54 Steven Rostedt 2019-10-11 18:20 ` Linus Torvalds 0 siblings, 1 reply; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 17:54 UTC (permalink / raw) To: LKML Cc: Matthew Garrett, jmorris, linux-security-module, linux-api, Ben Hutchings, Al Viro, Linus Torvalds [-- Attachment #1: Type: text/plain, Size: 7978 bytes --] [ Attached the reproducers to this email ] From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Running the latest kernel through my "make instances" stress tests, I triggered the following bug (with KASAN and kmemleak enabled): mkdir invoked oom-killer: gfp_mask=0x40cd0(GFP_KERNEL|__GFP_COMP|__GFP_RECLAIMABLE), order=0, oom_score_adj=0 CPU: 1 PID: 2229 Comm: mkdir Not tainted 5.4.0-rc2-test #325 Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014 Call Trace: dump_stack+0x64/0x8c dump_header+0x43/0x3b7 ? trace_hardirqs_on+0x48/0x4a oom_kill_process+0x68/0x2d5 out_of_memory+0x2aa/0x2d0 __alloc_pages_nodemask+0x96d/0xb67 __alloc_pages_node+0x19/0x1e alloc_slab_page+0x17/0x45 new_slab+0xd0/0x234 ___slab_alloc.constprop.86+0x18f/0x336 ? alloc_inode+0x2c/0x74 ? irq_trace+0x12/0x1e ? tracer_hardirqs_off+0x1d/0xd7 ? __slab_alloc.constprop.85+0x21/0x53 __slab_alloc.constprop.85+0x31/0x53 ? __slab_alloc.constprop.85+0x31/0x53 ? alloc_inode+0x2c/0x74 kmem_cache_alloc+0x50/0x179 ? alloc_inode+0x2c/0x74 alloc_inode+0x2c/0x74 new_inode_pseudo+0xf/0x48 new_inode+0x15/0x25 tracefs_get_inode+0x23/0x7c ? lookup_one_len+0x54/0x6c tracefs_create_file+0x53/0x11d trace_create_file+0x15/0x33 event_create_dir+0x2a3/0x34b __trace_add_new_event+0x1c/0x26 event_trace_add_tracer+0x56/0x86 trace_array_create+0x13e/0x1e1 instance_mkdir+0x8/0x17 tracefs_syscall_mkdir+0x39/0x50 ? get_dname+0x31/0x31 vfs_mkdir+0x78/0xa3 do_mkdirat+0x71/0xb0 sys_mkdir+0x19/0x1b do_fast_syscall_32+0xb0/0xed 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. Instead of allocating the proxy_ops (and then having to free it), I created a static proxy_ops. As tracefs only uses a subset of all the file_operations methods, that subset can be defined in the static proxy_ops, and then the passed in fops during the creation of the inode is saved in the dentry, and that is use to call the real functions by the proxy_ops. Fixes: ccbd54ff54e8 ("tracefs: Restrict tracefs when the kernel is locked down") Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- fs/tracefs/inode.c | 153 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 135 insertions(+), 18 deletions(-) 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); -- 2.20.1 [-- Attachment #2: ftrace-test-mkinstances --] [-- Type: application/octet-stream, Size: 1000 bytes --] #!/bin/bash tracefs=`cat /proc/mounts |grep tracefs| head -1 | cut -d' ' -f2` if [ -z "$tracefs" ]; then echo "tracefs not mounted" exit 0 fi if [ ! -d $tracefs/instances ]; then echo "No instances directory" exit 0 fi cd $tracefs/instances mkdir x rmdir x result=$? if [ $result -ne 0 ]; then echo "instance rmdir not supported, skipping this test" exit 0 fi instance_slam() { while :; do mkdir x mkdir y mkdir z rmdir x rmdir y rmdir z done 2>/dev/null } instance_slam & p1=$! echo $p1 instance_slam & p2=$! echo $p2 instance_slam & p3=$! echo $p3 instance_slam & p4=$! echo $p4 instance_slam & p5=$! echo $p5 for i in `seq 10`; do ls sleep 1 done kill -1 $p1 kill -1 $p2 kill -1 $p3 kill -1 $p4 kill -1 $p5 echo "Wait for processes to finish" wait $p1 $p2 $p3 $p4 $p5 echo "all processes finished, wait for cleanup" sleep 2 mkdir x y z ls x y z rmdir x y z for d in x y z; do if [ -d $d ]; then echo $d still exists exit -1 fi done echo SUCCESS exit 0 [-- Attachment #3: ftrace-test-mkinstances-2 --] [-- Type: application/octet-stream, Size: 890 bytes --] #!/bin/bash tracefs=`cat /proc/mounts |grep tracefs| head -1 | cut -d' ' -f2` if [ -z "$tracefs" ]; then echo "tracefs not mounted" exit 0 fi if [ ! -d $tracefs/instances ]; then echo "No instances directory" exit 0 fi cd $tracefs/instances instance_slam() { while :; do mkdir foo &> /dev/null rmdir foo &> /dev/null done } instance_read() { while :; do cat foo/trace &> /dev/null done } instance_set() { while :; do echo 1 > foo/events/sched/sched_switch done 2> /dev/null } instance_slam & x=`jobs -l` p1=`echo $x | cut -d' ' -f2` echo $p1 instance_set & x=`jobs -l | tail -1` p2=`echo $x | cut -d' ' -f2` echo $p2 sleep 10 kill -1 $p1 kill -1 $p2 echo "Wait for processes to finish" wait $p1 $p2 echo "all processes finished, wait for cleanup" sleep 2 mkdir foo ls foo rmdir foo if [ -d foo ]; then echo foo still exists exit -1 fi echo SUCCESS exit 0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 17:54 [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown Steven Rostedt @ 2019-10-11 18:20 ` Linus Torvalds 2019-10-11 18:36 ` Steven Rostedt 2019-10-11 20:25 ` Steven Rostedt 0 siblings, 2 replies; 13+ messages in thread From: Linus Torvalds @ 2019-10-11 18:20 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 18:20 ` Linus Torvalds @ 2019-10-11 18:36 ` Steven Rostedt 2019-10-11 19:24 ` Linus Torvalds ` (2 more replies) 2019-10-11 20:25 ` Steven Rostedt 1 sibling, 3 replies; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 18:36 UTC (permalink / raw) To: Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 18:36 ` Steven Rostedt @ 2019-10-11 19:24 ` Linus Torvalds 2019-10-11 19:50 ` Ben Hutchings 2019-10-11 21:46 ` Florian Weimer 2 siblings, 0 replies; 13+ messages in thread From: Linus Torvalds @ 2019-10-11 19:24 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 18:36 ` Steven Rostedt 2019-10-11 19:24 ` Linus Torvalds @ 2019-10-11 19:50 ` Ben Hutchings 2019-10-11 21:46 ` Florian Weimer 2 siblings, 0 replies; 13+ messages in thread From: Ben Hutchings @ 2019-10-11 19:50 UTC (permalink / raw) To: Steven Rostedt, Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Al Viro [-- Attachment #1: Type: text/plain, Size: 724 bytes --] 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. -- Ben Hutchings It is easier to change the specification to fit the program than vice versa. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 18:36 ` Steven Rostedt 2019-10-11 19:24 ` Linus Torvalds 2019-10-11 19:50 ` Ben Hutchings @ 2019-10-11 21:46 ` Florian Weimer 2019-10-11 22:27 ` Steven Rostedt 2 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2019-10-11 21:46 UTC (permalink / raw) To: Steven Rostedt Cc: Linus Torvalds, LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro * 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 21:46 ` Florian Weimer @ 2019-10-11 22:27 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 22:27 UTC (permalink / raw) To: Florian Weimer Cc: Linus Torvalds, LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 18:20 ` Linus Torvalds 2019-10-11 18:36 ` Steven Rostedt @ 2019-10-11 20:25 ` Steven Rostedt 2019-10-11 20:46 ` Linus Torvalds 2019-10-11 20:54 ` Steven Rostedt 1 sibling, 2 replies; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 20:25 UTC (permalink / raw) To: Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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); } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 20:25 ` Steven Rostedt @ 2019-10-11 20:46 ` Linus Torvalds 2019-10-11 21:08 ` Steven Rostedt 2019-10-11 20:54 ` Steven Rostedt 1 sibling, 1 reply; 13+ messages in thread From: Linus Torvalds @ 2019-10-11 20:46 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 20:46 ` Linus Torvalds @ 2019-10-11 21:08 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 21:08 UTC (permalink / raw) To: Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 20:25 ` Steven Rostedt 2019-10-11 20:46 ` Linus Torvalds @ 2019-10-11 20:54 ` Steven Rostedt 2019-10-11 21:00 ` Linus Torvalds 1 sibling, 1 reply; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 20:54 UTC (permalink / raw) To: Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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); } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 20:54 ` Steven Rostedt @ 2019-10-11 21:00 ` Linus Torvalds 2019-10-11 21:11 ` Steven Rostedt 0 siblings, 1 reply; 13+ messages in thread From: Linus Torvalds @ 2019-10-11 21:00 UTC (permalink / raw) To: Steven Rostedt Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown 2019-10-11 21:00 ` Linus Torvalds @ 2019-10-11 21:11 ` Steven Rostedt 0 siblings, 0 replies; 13+ messages in thread From: Steven Rostedt @ 2019-10-11 21:11 UTC (permalink / raw) To: Linus Torvalds Cc: LKML, Matthew Garrett, James Morris James Morris, LSM List, Linux API, Ben Hutchings, Al Viro 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-11 22:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-11 17:54 [PATCH] tracefs: Do not allocate and free proxy_ops for lockdown Steven Rostedt 2019-10-11 18:20 ` Linus Torvalds 2019-10-11 18:36 ` Steven Rostedt 2019-10-11 19:24 ` Linus Torvalds 2019-10-11 19:50 ` Ben Hutchings 2019-10-11 21:46 ` Florian Weimer 2019-10-11 22:27 ` Steven Rostedt 2019-10-11 20:25 ` Steven Rostedt 2019-10-11 20:46 ` Linus Torvalds 2019-10-11 21:08 ` Steven Rostedt 2019-10-11 20:54 ` Steven Rostedt 2019-10-11 21:00 ` Linus Torvalds 2019-10-11 21:11 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).