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