Message-ID: <CAHC9VhQhCvZ3KRHM=sx4cprCEgYanuPT18mzB8rtnVU5urhhwg@mail.gmail.com>
Date:   Tue, 30 May 2017 17:21:39 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-audit@...hat.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: 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@...hat.com> 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@...hat.com>
> ---
>  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
www.paul-moore.com

Powered by blists - more mailing lists