Message-ID: <20181119225856.dt3l7qzg2ftggon4@madcap2.tricolour.ca>
Date:   Mon, 19 Nov 2018 17:58:56 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     linux-fsdevel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        linux-kernel@...r.kernel.org, linux-audit@...hat.com,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...hat.com>, sgrubb@...hat.com
Subject: Re: [RFC PATCH ghak100 V1 1/2] audit: avoid fcaps on MNT_FORCE

On 2018-11-19 13:47, Miklos Szeredi wrote:
> On Fri, Nov 16, 2018 at 6:34 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> >
> > Don't fetch fcaps when umount2 is called with MNT_FORCE to avoid a
> > process hang while it waits for the missing resource to (possibly never)
> > re-appear.
> 
> The patch would be pretty good if the dependence on MNT_FORCE wasn't
> added.   As it is, it's buggy in more ways than one:
> 
>  - It does the opposite of the above (i.e. skips fcaps *unless*
> MNT_FORCE is set)

I agree it looks wrong now that I look at it.  It turns out my test case
didn't trigger it properly since "umount -l" doesn't set MNT_FORCE while
it needs "-f" to do so.  This is unacceptable since "-l" needs to work
in this situation too.

>  - sets LOOKUP_NO_REVAL from caller of path lookup, which is invalid
> (LOOKUP_NO_REVAL is used only internally by path lookup)

Fair enough.  949a852e46dd viro 2016-03-14 ("namei: teach lookup_slow()
to skip revalidate") needs a comment update.

Maybe my patch was interacting with this one and changing the behaviour I
expected.

>  - the fact that *_path_mountpoint_at() shouldn't touch the mount root
> is independent of MNT_FORCE

We don't entirely agree here since I'm still aiming for a best effort to
collect this information for the PATH record, but that may be misleading
at best.

> I still don't quite understand what audit is trying to do here, but
> apparently it's okay to skip getxattr in the MNT_FORCE case.  So why
> is it not okay to skip it in the non-MNT_FORCE case?

The simple answer is that the audit PATH record format expects the four
cap_f* fields to be there and a best effort is being attempted to fill
in that information in an expected way with meaningful values.  Perhaps
better to accept that it is unreasonable to expect any fcaps on any
umount operation and simply ignore those fields in the PATH record for
umount syscall events.

This is really a problem the audit folks have backed ourselves into.
This was introduced by 851f7ff56d9c ("This patch will print
cap_permitted and cap_inheritable data in the PATH...")
The fcaps are only really needed for the case of an event that changes
fcaps.  In that case, the fcaps should have been added as a seperate
audit record to accompany this event as necessary, rather than included
in the PATH record that is shared by multiple event types, most of which
do not change the fcaps.  There has been significant and ongoing effort
to normalize all audit record types so that they contain predictable
fields in a predictable order without any fields that swing in and out
since this makes userspace audit record parsers faster and more
reliable.

My preferred solution would be to in fact remove these four cap_f*
fields from the PATH record and put them in a new record that was only
included when the event is relevant and the values are non-zero.
This isn't an option with current upstream kernel audit devel policy.

Thanks Miklos for taking the time to provide feedback on this patch.

> Thanks,
> Miklos

- 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

Powered by blists - more mailing lists