Re: [PATCH RFC 5/5] fs: allow to tuck mounts explicitly

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, Mar 18, 2023 at 04:52:01PM +0100, Christian Brauner wrote:

> +/**
> + * mnt_tuck_mountpoint - tuck a mount beneath another one
> + *
> + * @tucked_mnt: the mount to tuck
> + * @top_mnt:	the mount to tuck @tucked_mnt under
> + * @tucked_mp:	the new mountpoint of @top_mnt on @tucked_mnt
> + *
> + * Remove @top_mnt from its current mountpoint @top_mnt->mnt_mp and
> + * parent @top_mnt->mnt_parent and mount it on top of @tucked_mnt at
> + * @tucked_mp. And mount @tucked_mnt on the old parent and old
> + * mountpoint of @top_mnt.
> + *
> + * Note that we keep the reference count in tact when we remove @top_mnt
> + * from its old mountpoint and parent to prevent UAF issues. Once we've
> + * mounted @top_mnt on @tucked_mnt the reference count gets bumped once
> + * more. So make sure that we drop it to not leak the mount and
> + * mountpoint.
> + */
> +static void mnt_tuck_mountpoint(struct mount *tucked_mnt, struct mount *top_mnt,
> +				struct mountpoint *tucked_mp)
> +{
> +	struct mount *old_top_parent = top_mnt->mnt_parent;
> +	struct mountpoint *old_top_mp;
> +
> +	old_top_mp = unhash_mnt(top_mnt);
> +	attach_mnt(top_mnt, tucked_mnt, tucked_mp);
> +	mnt_set_mountpoint(old_top_parent, old_top_mp, tucked_mnt);
> +	put_mountpoint(old_top_mp);
> +	mnt_add_count(old_top_parent, -1);
> +}

Umm...  And if something is mounted on top of your tucked_mnt?  Right
on top of its root, I mean...  BTW, why not make it

static void mnt_tuck_mountpoint(struct mount *tucked_mnt, struct mount *top_mnt,
                                struct mountpoint *tucked_mp)
{
        struct mount *old_parent = top_mnt->mnt_parent;
        struct mountpoint *old_mp = top_mnt->mnt_mp;

        mnt_set_mountpoint(old_parent, old_mp, tucked_mnt);
        mnt_change_mountpoint(tucked_mnt, tucked_mp, top_mnt);
}

I mean, look at the existing caller of mnt_change_mountpoint() nearby -
that's precisely how that "tucking" is done right now.  And I'm rather
afraid that it's vulnerable to the same problem - if the tree we are
copying has the root of its root mount overmounted by something...

> +static void tuck_mnt(struct mount *tucked_mnt,
> +		     struct mount *top_mnt,
> +		     struct mountpoint *tucked_mp)
> +{
> +	mnt_tuck_mountpoint(tucked_mnt, top_mnt, tucked_mp);
> +	__attach_mnt(tucked_mnt, tucked_mnt->mnt_parent);
> +}

Not sure it's worth bothering with - only one caller and it might be
cleaner to expand attach_mnt() there, turning it into set or tuck,
followed by unconditional __attach...

> +typedef enum mnt_tree_flags_t {
> +	MNT_TREE_MOVE	= BIT(0),
> +	MNT_TREE_TUCK	= BIT(1),
> +} mnt_tree_flags_t;

What combinations are possible?

>  static int attach_recursive_mnt(struct mount *source_mnt,
> -			struct mount *dest_mnt,
> -			struct mountpoint *dest_mp,
> -			bool moving)
> +				struct mount *top_mnt,
> +				struct mountpoint *dest_mp,
> +				mnt_tree_flags_t flags)
>  {
>  	struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns;
>  	HLIST_HEAD(tree_list);
> -	struct mnt_namespace *ns = dest_mnt->mnt_ns;
> +	struct mnt_namespace *ns = top_mnt->mnt_ns;
>  	struct mountpoint *smp;
> -	struct mount *child, *p;
> +	struct mount *child, *dest_mnt, *p;
>  	struct hlist_node *n;
> -	int err;
> +	int err = 0;
> +	bool moving = flags & MNT_TREE_MOVE, tuck = flags & MNT_TREE_TUCK;
>  
>  	/* Preallocate a mountpoint in case the new mounts need
>  	 * to be tucked under other mounts.
> @@ -2244,29 +2305,48 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>  			goto out;
>  	}
>  
> +	if (tuck)
> +		dest_mnt = top_mnt->mnt_parent;
> +	else
> +		dest_mnt = top_mnt;
> +
>  	if (IS_MNT_SHARED(dest_mnt)) {
>  		err = invent_group_ids(source_mnt, true);
>  		if (err)
>  			goto out;
>  		err = propagate_mnt(dest_mnt, dest_mp, source_mnt, &tree_list);
> -		lock_mount_hash();
> -		if (err)
> -			goto out_cleanup_ids;
> +	}
> +	lock_mount_hash();
> +	if (err)
> +		goto out_cleanup_ids;
> +
> +	/* Recheck with lock_mount_hash() held. */
> +	if (tuck && IS_MNT_LOCKED(top_mnt)) {
> +		err = -EINVAL;
> +		goto out_cleanup_ids;
> +	}
> +
> +	if (IS_MNT_SHARED(dest_mnt)) {
>  		for (p = source_mnt; p; p = next_mnt(p, source_mnt))
>  			set_mnt_shared(p);
> -	} else {
> -		lock_mount_hash();
>  	}
> +
>  	if (moving) {
>  		unhash_mnt(source_mnt);
> -		attach_mnt(source_mnt, dest_mnt, dest_mp);
> +		if (tuck)
> +			tuck_mnt(source_mnt, top_mnt, smp);
> +		else
> +			attach_mnt(source_mnt, dest_mnt, dest_mp);
>  		touch_mnt_namespace(source_mnt->mnt_ns);
>  	} else {
>  		if (source_mnt->mnt_ns) {
>  			/* move from anon - the caller will destroy */
>  			list_del_init(&source_mnt->mnt_ns->list);
>  		}
> -		mnt_set_mountpoint(dest_mnt, dest_mp, source_mnt);
> +		if (tuck)
> +			mnt_tuck_mountpoint(source_mnt, top_mnt, smp);
> +		else
> +			mnt_set_mountpoint(dest_mnt, dest_mp, source_mnt);
>  		commit_tree(source_mnt);
>  	}
>  
> @@ -2306,14 +2386,35 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>  	return err;
>  }
>  
> -static struct mountpoint *lock_mount(struct path *path)
> +/**
> + * lock_mount_mountpoint - lock mount and mountpoint
> + * @path: target path
> + * @tuck: whether we intend to tuck a mount beneath @path
> + *
> + * Follow the mount stack on @path until the top mount is found.
> + *
> + * If we intend to mount on top of @path->mnt acquire the inode_lock()
> + * for the top mount's ->mnt_root to protect against concurrent removal
> + * of our prospective mountpoint from another mount namespace.
> + *
> + * If we intend to tuck beneath the top mount @m acquire the
> + * inode_lock() on @m's mountpoint @mp on @m->mnt_parent. Otherwise we
> + * risk racing with someone who unlinked @mp from another mount
> + * namespace where @m doesn't have a child mount mounted @mp. We don't
> + * care if @m->mnt_root/@path->dentry is removed (as long as
> + * @path->dentry isn't equal to @m->mnt_mountpoint of course).
> + *
> + * Return: Either the target mountpoint on the top mount or the top
> + *         mount's mountpoint.
> + */
> +static struct mountpoint *lock_mount_mountpoint(struct path *path, bool tuck)
>  {
>  	struct vfsmount *mnt = path->mnt;
>  	struct dentry *dentry;
>  	struct mountpoint *mp;
>  
>  	for (;;) {
> -		dentry = path->dentry;
> +		dentry = tuck ? real_mount(mnt)->mnt_mountpoint : path->dentry;
>  		inode_lock(dentry->d_inode);

What happens if another mount --move changes ->mnt_mountpoint of that sucker
just as we fetch it and we end up with inode_lock(dentry->d_inode) of a dentry
that is not pinned by anything?  path->dentry *is* pinned, so it's safe to
work with; this one, OTOH...

> +/**
> + * can_tuck_mount - check that we can tuck a mount
> + * @from: mount to tuck under
> + * @to:   mount under which to tuck
> + *
> + * - Make sure that the mount to tuck under isn't a shared mount so we
> + *   force the kernel to allocate a new peer group id. This simplifies
> + *   the mount trees that can be created and limits propagation events
> + *   in cases where @to, and/or @to->mnt_parent are in the same peer
> + *   group. Something that's a nuisance already today.
> + * - Make sure that @to->dentry is actually the root of a mount under
> + *   which we can tuck another mount.
> + * - Make sure that nothing can be tucked under the caller's current
> + *   root or the rootfs of the namespace.
> + * - Make sure that the caller can unmount the topmost mount ensuring
> + *   that the caller could reveal the underlying mountpoint.
> + *
> + * Return: On success 0, and on error a negative error code is returned.
> + */
> +static int can_tuck_mount(struct path *from, struct path *to)
> +{
> +	struct mount *mnt_from = real_mount(from->mnt),
> +		     *mnt_to = real_mount(to->mnt);
> +
> +	if (!check_mnt(mnt_to))
> +		return -EINVAL;
> +
> +	if (!mnt_has_parent(mnt_to))
> +		return -EINVAL;
> +
> +	if (IS_MNT_SHARED(mnt_from))
> +		return -EINVAL;

What if it's not shared, but gets propagation from whatever we
are going to attach it to?

> +	if (!path_mounted(to))
> +		return -EINVAL;
> +
> +	if (mnt_from == mnt_to)
> +		return -EINVAL;
> +
> +	/*
> +	 * Tucking a mount beneath the rootfs only makes sense when the
> +	 * tuck semantics of pivot_root(".", ".") are used.
> +	 */
> +	if (&mnt_to->mnt == current->fs->root.mnt)
> +		return -EINVAL;
> +	if (mnt_to->mnt_parent == current->nsproxy->mnt_ns->root)
> +		return -EINVAL;
> +
> +	for (struct mount *p = mnt_from; mnt_has_parent(p); p = p->mnt_parent)
> +		if (p == mnt_to)
> +			return -EINVAL;

Umm...  Can we have !mnt_has_parent(mnt_from), BTW?

> +	/* Ensure the caller could reveal the underlying mount. */
> +	return can_umount(to, 0);

That's really odd.  It duplicates quite a bit of the tests above and
the whole thing is rather hard to follow, especially since you've got
tests done after the can_tuck_mount() call in do_move_mount()...

> +}
> +
> +static int do_move_mount(struct path *old_path, struct path *new_path,
> +			 bool tuck)
>  {
>  	struct mnt_namespace *ns;
>  	struct mount *p;
> @@ -2858,8 +3021,9 @@ static int do_move_mount(struct path *old_path, struct path *new_path)
>  	struct mountpoint *mp, *old_mp;
>  	int err;
>  	bool attached;
> +	mnt_tree_flags_t flags = 0;
>  
> -	mp = lock_mount(new_path);
> +	mp = lock_mount_mountpoint(new_path, tuck);
>  	if (IS_ERR(mp))
>  		return PTR_ERR(mp);
>  
> @@ -2867,9 +3031,20 @@ static int do_move_mount(struct path *old_path, struct path *new_path)
>  	p = real_mount(new_path->mnt);
>  	parent = old->mnt_parent;
>  	attached = mnt_has_parent(old);
> +	if (attached)
> +		flags |= MNT_TREE_MOVE;
>  	old_mp = old->mnt_mp;
>  	ns = old->mnt_ns;
>  
> +	if (tuck) {
> +		err = can_tuck_mount(old_path, new_path);
> +		if (err)
> +			goto out;
> +
> +		p = p->mnt_parent;
> +		flags |= MNT_TREE_TUCK;
> +	}
> +
>  	err = -EINVAL;
>  	/* The mountpoint must be in our namespace. */
>  	if (!check_mnt(p))

Why not check it first?  That'd kill the corresponding
check_mnt(to) in can_tuck_mount() (check_mnt() on parent is
the same as on child).  Confused...

> @@ -2910,8 +3085,7 @@ static int do_move_mount(struct path *old_path, struct path *new_path)
>  		if (p == old)
>  			goto out;
>  
> -	err = attach_recursive_mnt(old, real_mount(new_path->mnt), mp,
> -				   attached);
> +	err = attach_recursive_mnt(old, real_mount(new_path->mnt), mp, flags);
>  	if (err)
>  		goto out;
>  
> @@ -2943,7 +3117,7 @@ static int do_move_mount_old(struct path *path, const char *old_name)
>  	if (err)
>  		return err;
>  
> -	err = do_move_mount(&old_path, path);
> +	err = do_move_mount(&old_path, path, false);
>  	path_put(&old_path);
>  	return err;
>  }
> @@ -3807,6 +3981,10 @@ SYSCALL_DEFINE5(move_mount,
>  	if (flags & ~MOVE_MOUNT__MASK)
>  		return -EINVAL;
>  
> +	if ((flags & (MOVE_MOUNT_TUCK | MOVE_MOUNT_SET_GROUP)) ==
> +	    (MOVE_MOUNT_TUCK | MOVE_MOUNT_SET_GROUP))
> +		return -EINVAL;
> +
>  	/* If someone gives a pathname, they aren't permitted to move
>  	 * from an fd that requires unmount as we can't get at the flag
>  	 * to clear it afterwards.
> @@ -3836,7 +4014,8 @@ SYSCALL_DEFINE5(move_mount,
>  	if (flags & MOVE_MOUNT_SET_GROUP)
>  		ret = do_set_group(&from_path, &to_path);
>  	else
> -		ret = do_move_mount(&from_path, &to_path);
> +		ret = do_move_mount(&from_path, &to_path,
> +				    (flags & MOVE_MOUNT_TUCK));

OK, so you want to be able to use that on an anonymous tree, then?