From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751703AbdF1TDP (ORCPT ); Wed, 28 Jun 2017 15:03:15 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:36023 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751495AbdF1TDI (ORCPT ); Wed, 28 Jun 2017 15:03:08 -0400 MIME-Version: 1.0 X-Originating-IP: [108.49.102.27] In-Reply-To: <20170627211138.GE27193@madcap2.tricolour.ca> References: <20170627211138.GE27193@madcap2.tricolour.ca> From: Paul Moore Date: Wed, 28 Jun 2017 15:03:05 -0400 Message-ID: Subject: Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents To: Richard Guy Briggs Cc: linux-kernel@vger.kernel.org, linux-audit@redhat.com, Greg Kroah-Hartman , Steven Rostedt , Ingo Molnar , Al Viro Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs wrote: > On 2017-05-30 17:21, Paul Moore wrote: >> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs 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 >> > #include >> > #include >> > +#include >> > >> > #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