Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents

On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Tracefs or debugfs were causing hundreds to thousands of null PATH > records to be associated with the init_module and finit_module SYSCALL > records on a few modules when the following rule was in place for > startup: > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load > > This happens because the parent inode is not found in the task's > audit_names list and hence treats it as anonymous. This gives us no > information other than a numerical device number that may no longer be > visible upon log inspeciton, and an inode number. > > Fill in the filesystem type, filesystem magic number and full pathname > from the filesystem mount point on previously null PATH records from > entries that have an anonymous parent from the child dentry using > dentry_path_raw(). > > Make the dentry argument of __audit_inode_child() non-const so that we > can take a reference to it in the case of an anonymous parent with > dget() and dget_parent() to be able to later print a partial path from > the host filesystem rather than null. > > Since all we are given is an inode of the parent and the dentry of the > child, finding the path from the mount point to the root of the > filesystem is more challenging that would involve searching all > vfsmounts from "/" until a matching dentry is found for that > filesystem's root dentry. Even if one is found, there may be more than > one mount point. At this point the gain seems marginal since > knowing the filesystem type and path are a significant help in tracking > down the source of the PATH records and being to address them. > > Sample output: > type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 > type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE > type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT > ... > type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE > type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4" > type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load" > > See: https://github.com/linux-audit/audit-kernel/issues/8 > Test case: https://github.com/linux-audit/audit-testsuite/issues/42 > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > include/linux/audit.h | 8 ++++---- > kernel/audit.c | 16 ++++++++++++++++ > kernel/audit.h | 1 + > kernel/auditsc.c | 8 +++++++- > 4 files changed, 28 insertions(+), 5 deletions(-) ... > diff --git a/kernel/audit.c b/kernel/audit.c > index 25dd70a..7d83c5a 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -66,6 +66,7 @@ > #include <linux/freezer.h> > #include <linux/pid_namespace.h> > #include <net/netns/generic.h> > +#include <linux/dcache.h> > > #include "audit.h" > > @@ -1884,6 +1885,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry, > name->gid = inode->i_gid; > name->rdev = inode->i_rdev; > security_inode_getsecid(inode, &name->osid); > + if (name->dentry) { > + dput(name->dentry); > + name->dentry = NULL; > + } Out of curiosity, what terrible things happen if we take a reference to a non-NULL dentry passed to audit_copy_inode() and store it in name->dentry? Does performance tank?

Also out of curiosity, why do we want to drop a dentry reference here if one already exists?

> audit_copy_fcaps(name, dentry); > } > > @@ -1925,6 +1930,17 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, > audit_log_n_untrustedstring(ab, n->name->name, > n->name_len); > } > + } else if (n->dentry) { > + char *fullpath; > + const char *fullpathp; > + > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL); > + if (!fullpath) > + return; I'm wondering if there is some value in still emitting the record if the kmalloc() fails, just with the name field set as the unset "?" value, e.g. "name=?". Thoughts?

paul moore