Re: [PATCH 3/4] use uniform permission checks for all mount propagation changes

[Andrei Vagin <avagin@google.com>]

On Fri, Aug 15, 2025 at 4:35 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> do_change_type() and do_set_group() are operating on different
> aspects of the same thing - propagation graph.  The latter
> asks for mounts involved to be mounted in namespace(s) the caller
> has CAP_SYS_ADMIN for.  The former is a mess - originally it
> didn't even check that mount *is* mounted.  That got fixed,
> but the resulting check turns out to be too strict for userland -
> in effect, we check that mount is in our namespace, having already
> checked that we have CAP_SYS_ADMIN there.
>
> What we really need (in both cases) is
>         * we only touch mounts that are mounted.  Hard requirement,
> data corruption if that's get violated.
>         * we don't allow to mess with a namespace unless you already
> have enough permissions to do so (i.e. CAP_SYS_ADMIN in its userns).
>
> That's an equivalent of what do_set_group() does; let's extract that
> into a helper (may_change_propagation()) and use it in both
> do_set_group() and do_change_type().
>

Al, thank you for the fix.

Acked-by: Andrei Vagin <avagin@gmail.com>

> Fixes: 12f147ddd6de "do_change_type(): refuse to operate on unmounted/not ours mounts"
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/namespace.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 1c97f93d1865..88db58061919 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2859,6 +2859,19 @@ static int graft_tree(struct mount *mnt, struct mount *p, struct mountpoint *mp)
>         return attach_recursive_mnt(mnt, p, mp);
>  }
>
> +static int may_change_propagation(const struct mount *m)
> +{
> +        struct mnt_namespace *ns = m->mnt_ns;
> +
> +        // it must be mounted in some namespace
> +        if (IS_ERR_OR_NULL(ns))         // is_mounted()
> +                return -EINVAL;
> +        // and the caller must be admin in userns of that namespace
> +        if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN))
> +                return -EPERM;
> +        return 0;
> +}
> +
>  /*
>   * Sanity check the flags to change_mnt_propagation.
>   */
> @@ -2895,10 +2908,10 @@ static int do_change_type(struct path *path, int ms_flags)
>                 return -EINVAL;
>
>         namespace_lock();
> -       if (!check_mnt(mnt)) {
> -               err = -EINVAL;
> +       err = may_change_propagation(mnt);
> +       if (err)
>                 goto out_unlock;
> -       }
> +
>         if (type == MS_SHARED) {
>                 err = invent_group_ids(mnt, recurse);
>                 if (err)
> @@ -3344,18 +3357,11 @@ static int do_set_group(struct path *from_path, struct path *to_path)
>
>         namespace_lock();
>
> -       err = -EINVAL;
> -       /* To and From must be mounted */
> -       if (!is_mounted(&from->mnt))
> -               goto out;
> -       if (!is_mounted(&to->mnt))
> -               goto out;
> -
> -       err = -EPERM;
> -       /* We should be allowed to modify mount namespaces of both mounts */
> -       if (!ns_capable(from->mnt_ns->user_ns, CAP_SYS_ADMIN))
> +       err = may_change_propagation(from);
> +       if (err)
>                 goto out;
> -       if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
> +       err = may_change_propagation(to);
> +       if (err)
>                 goto out;
>
>         err = -EINVAL;
> --
> 2.47.2
>