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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhSGSw0Ke+JuW8s9Lszv7HW=1M0ro7rVqxM=DGKHSfQ0mA@mail.gmail.com>
Date:   Wed, 28 Jun 2017 15:03:05 -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, Jun 27, 2017 at 5:11 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2017-05-30 17:21, Paul Moore wrote:
>> On Tue, Apr 4, 2017 at 5:21 AM, Richard Guy Briggs <rgb@...hat.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