Messages in this thread | | | From | Paul Moore <> | Date | Wed, 28 Jun 2017 15:03:05 -0400 | Subject | 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@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@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
| |