Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents
On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2017-05-30 17:21, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
...
> > 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?
Interesting idea. Right now it is optimized to only take a reference to
the dentry's parent dentry in the case we're handed an anonymous entry.
Most of the time it will never be used even though we invest in the
overhead of taking a reference count. Besides, __audit_inode_child()
hands in a NULL for the dentry parameter to audit_copy_inode().
[NOTE: audit_copy_inode() hands a NULL dentry only in the anonymous parent case]
I believe I was just thinking of less conditional handling, especially
when reference counts are concerned. I'm just trying to limit future
headaches, but I suspect the perf cost would be problematic, and as
you point out, there is no *need* for the majority of cases.
Looking at this again today, why would we want to clear name->dentry
in audit_copy_inode() if it is already set? Does that ever happen?
I'm not sure it does ...
I'm
assuming you are hinting at also using that dentry to compare the
audit_names entry, which I think it a bad idea since there could be
multiple paths to access a dentry. I did orignially have another patch
that would have tried to use that as well, which didn't seem to hurt,
but I didn't think was worth upstreaming.
No, I wasn't thinking that, the dev/inode numbers should be sufficient
in those cases I believe; I'm not sure the dentry would help us here.
> Also out of curiosity, why do we want to drop a dentry reference
here
> if one already exists?
I think we want to drop a dentry reference here because this inode child
could be a subsequent access to the same dentry with a full path,
removing the need to cache this dentry information in the first place.
Related to my comment above from today ... what code path please?
> > @@ -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?
Possibly. We've got much bigger problems if that happens, but this
sounds like a good defensive coding approach. I'm even tempted to call
audit_panic().
No audit_panic(). We've still got good information that we can
record, e.g. dev/inode numbers; let's just print "name=?" and go on
our way recording the rest of the information. This is in keeping
with the current audit_log_name() error handling.
At the very least you need to clean up here instead of just returning.
As the patch currently stands I believe this will end up leaking an
audit_buffer.
--
paul moore
www.paul-moore.com