On Wed, Dec 06, 2023 at 09:24:45PM +0100, Miklos Szeredi wrote:

On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn [off-list ref] wrote:

quoted

On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:

quoted

quoted

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