linux-kernel - Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount

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] [day] [month] [year] [list]

Message-ID: <20190201224912.hfhdvzm7yqr266pa@madcap2.tricolour.ca>
Date:   Fri, 1 Feb 2019 17:49:12 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Nathan Chancellor <natechancellor@...il.com>,
        linux-fsdevel@...r.kernel.org, viro@...iv.linux.org.uk,
        LKML <linux-kernel@...r.kernel.org>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        Steve Grubb <sgrubb@...hat.com>, Eric Paris <eparis@...hat.com>
Subject: Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount

On 2019-02-01 16:57, Richard Guy Briggs wrote:
> On 2019-02-01 16:05, Paul Moore wrote:
> > On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
> > <natechancellor@...il.com> wrote:
> > > On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > > it waits for the missing resource to (possibly never) re-appear.
> > > >
> > > > Note the comment above user_path_mountpoint_at():
> > > >  * A umount is a special case for path walking. We're not actually interested
> > > >  * in the inode in this situation, and ESTALE errors can be a problem.  We
> > > >  * simply want track down the dentry and vfsmount attached at the mountpoint
> > > >  * and avoid revalidating the last component.
> > > >
> > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > > >
> > > > Please see the github issue tracker
> > > > https://github.com/linux-audit/audit-kernel/issues/100
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > > > ---
> > > >  fs/namei.c            |  2 +-
> > > >  fs/namespace.c        |  2 ++
> > > >  include/linux/audit.h | 15 ++++++++++-----
> > > >  include/linux/namei.h |  3 +++
> > > >  kernel/audit.c        | 10 +++++++++-
> > > >  kernel/audit.h        |  2 +-
> > > >  kernel/auditsc.c      |  6 +++---
> > > >  7 files changed, 29 insertions(+), 11 deletions(-)
> > 
> > ...
> > 
> > > >  /* Copy inode data into an audit_names. */
> > > >  void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > > > -                   struct inode *inode)
> > > > +                   struct inode *inode, unsigned int flags)
> > > >  {
> > > >       name->ino   = inode->i_ino;
> > > >       name->dev   = inode->i_sb->s_dev;
> > > > @@ -2120,6 +2124,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 (flags & AUDIT_INODE_NOEVAL) {
> > >
> > > I don't know if this has been reported or if I am missing something but
> > > on next-20190201, this line causes an error with arm allyesconfig (as
> > > CONFIG_AUDITSYCALL doesn't get selected):
> > 
> > ...
> > 
> > >   CC      kernel/audit.o
> > > kernel/audit.c: In function 'audit_copy_inode':
> > > kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> > >   if (flags & AUDIT_INODE_NOEVAL) {
> > >               ^~~~~~~~~~~~~~~~~~
> > >               AUDIT_TYPE_NORMAL
> > > kernel/audit.c:2130:14: note: each undeclared identifier is reported only once for each function it appears in
> > > make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> > > make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> > > make: *** [Makefile:296: __build_one_by_one] Error 2
> > 
> > I hadn't seen this reported to the audit list yet, thanks for letting us now.
> 
> Thanks Nathan for the report.
> 
> > Richard, please submit a patch to fix this ASAP.  Looking at this, the
> > obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
> > sure if that itself is going to cause problems (it doesn't look like
> > it).  Actually, thinking out loud, I wonder if we shouldn't move
> > audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
> > audit_log_name() too?
> 
> They have all been moved in ghak105 v2 patch 2 which is in your queue.
> Lemme think if there is a quicker simpler solution for this patch.
> That ghak105 v2.patch2 will have a number of merge conflicts and I've
> already sorted those out, so I'm willing to post a respin (v3) of it to
> make your life easier.  (audit_log_key() should also be moved, now that
> I check...)

The really simple quick and dirty fix, which is cheating, is to move
that AUDIT_INODE_NOEVAL definition 15 lines up in include/linux/audit.h
just above the #ifdef CONFIG_AUDITSYSCALL.

The right fix is in ghak105 v2.patch2 with the audit_context pointer
still in the audit_buffer.  Lemme rebase, fix that and repost...

> > paul moore
> 
> - RGB

- RGB

--
Richard Guy Briggs <rgb@...hat.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