Re: [PATCH 3/4] listmount: small changes in semantics

[Christian Brauner <brauner@kernel.org>]

On Wed, Dec 06, 2023 at 09:24:45PM +0100, Miklos Szeredi wrote:
> On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
> 
> > > -     if (!is_path_reachable(m, mnt->mnt_root, &rootmnt))
> > > -             return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
> > > +     if (!capable(CAP_SYS_ADMIN) &&
> >
> > Was there a reason to do the capable check first?  In general,
> > checking capable() when not needed is frowned upon, as it will
> > set the PF_SUPERPRIV flag.
> >
> 
> I synchronized the permission checking with statmount() without
> thinking about the order.   I guess we can change the order back in
> both syscalls?

I can just change the order. It's mostly a question of what is more
expensive. If there's such unpleasant side-effects... then sure I'll
reorder.

> I also don't understand the reason behind the using the _noaudit()
> variant.  Christian?

The reasoning is similar to the change in commit e7eda157c407 ("fs:
don't audit the capability check in simple_xattr_list()").

    "The check being unconditional may lead to unwanted denials reported by
    LSMs when a process has the capability granted by DAC, but denied by an
    LSM. In the case of SELinux such denials are a problem, since they can't
    be effectively filtered out via the policy and when not silenced, they
    produce noise that may hide a true problem or an attack."

So for system calls like listmount() that we can expect to be called a
lot of times (findmnt etc at some point) this would needlessly spam
dmesg without any value. We can always change that to an explicit
capable() later.