Looks good! Thanks!
Reviewed-by: Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx>
Tested-by: Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx>
(CRIU tests PASS for the whole patchset)
On Sat, Aug 16, 2025 at 7:35 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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().
>
> Fixes: 12f147ddd6de "do_change_type(): refuse to operate on unmounted/not ours mounts"
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
