Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676187 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with  anonymous parents Richard Guy Briggs <rgb@redhat.com> - 2017-06-27 23:20 +0200
    Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with  anonymous parents Paul Moore <paul@paul-moore.com> - 2017-06-28 21:10 +0200
      Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with  anonymous parents Richard Guy Briggs <rgb@redhat.com> - 2017-06-29 23:30 +0200
        Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries  with anonymous parents Steven Rostedt <rostedt@goodmis.org> - 2017-06-30 02:00 +0200
          Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with  anonymous parents Richard Guy Briggs <rgb@redhat.com> - 2017-06-30 15:10 +0200

#1676187 — Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents

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:
> > 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@redhat.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?

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().  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.

> 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.

> >         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?

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().

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[toc] | [next] | [standalone]

#1677082

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

[toc] | [prev] | [next] | [standalone]

#1678210

On 2017-06-28 15:03, Paul Moore wrote:
> 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.

I don't like the conditional either, but it appears to be so seldom
needed that it looks like the lesser of evils.

> 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 ...

It has been nearly 3 months since I coded that, so I'll have to dive in
and re-analyse what I was thinking at that time.  I think that rationale
was that if audit_copy_inode() is called again on that audit_name struct
that it could be called by audit_log_link_denied() or __audit_inode()
not needing the dentry reference or even by __audit_inode_child() and
have it replaced, needing a reference count correction.

So I'm gathering that you think we should just leave it there unless
that reference gets updated/replaced which would only happen in the case
of the child of an anonymous parent.  If the other methods succeed, that
reference will simply be ignored and freed when the context 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?

First reference to an anonymous parent, then a subsequent call
referencing its child.  Since the anonymous parent and its child don't
have a name stored, it can never match a subsequent parent lookup.

The more I look at this, I think I missed another bug elsewhere failing
to call audit_getname() from getname_flags() or getname_kernel() that is
causing this anonymous parent.

> >> > @@ -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.

Would you be ok with something in-line like "name=(error)" to indicate
the information is there but an outside influence prevented it from
being printed?  (The "?" was my addition.)

There *is* precedent and an existing method in audit_log_name() to panic.

Now that I look at the audit_log_name() patch again, I see a bug in
checking the result of dentry_path_raw().

> 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.

I now see there is a buffer leak that needs to be addressed too.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[toc] | [prev] | [next] | [standalone]

#1678302 — Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents

On Thu, 29 Jun 2017 17:21:22 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> > 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 ...  
> 
> It has been nearly 3 months since I coded that, so I'll have to dive in
> and re-analyse what I was thinking at that time.  I think that rationale
> was that if audit_copy_inode() is called again on that audit_name struct
> that it could be called by audit_log_link_denied() or __audit_inode()
> not needing the dentry reference or even by __audit_inode_child() and
> have it replaced, needing a reference count correction.
>

Just a note. If after 3 months you need to re-analyze, you either need
to design things simpler, or have better comments in the code.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1678805

On 2017-06-29 19:58, Steven Rostedt wrote:
> On Thu, 29 Jun 2017 17:21:22 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> > > 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 ...  
> > 
> > It has been nearly 3 months since I coded that, so I'll have to dive in
> > and re-analyse what I was thinking at that time.  I think that rationale
> > was that if audit_copy_inode() is called again on that audit_name struct
> > that it could be called by audit_log_link_denied() or __audit_inode()
> > not needing the dentry reference or even by __audit_inode_child() and
> > have it replaced, needing a reference count correction.
> 
> Just a note. If after 3 months you need to re-analyze, you either need
> to design things simpler, or have better comments in the code.

Yep, both occurred to me.  ;-)    Thanks Steve.

> -- Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web