[PATCH review 19/19] vfs: Do not allow escaping from bind mounts.

[Al Viro]

On Thu, Apr 02, 2015 at 08:56:23PM -0500, Eric W. Biederman wrote:
> +static void mark_violated_mounts(struct dentry *dentry, struct dentry *target)
> +{
> +	/* Mark all mountroots that are ancestors of dentry
> +	 * that do not share a common ancestor with target
> +	 *
> +	 * This function assumes both dentries are part of a DAG.
> +	 */
> +	struct dentry *p;
> +
> +	for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
> +		if (!d_mountroot(p))
> +			continue;
> +
> +		if (d_ancestor(p, target))
> +			break;

Egads...  You do realize that you'll keep walking the path from target to
root again and again?  It's a tree and we have already walked up to the root.
So we know the depths and tree topology can't change due to the rename_lock
being held by caller.

> +	if (!IS_ROOT(dentry) && !IS_ROOT(target)) {
> +		mark_violated_mounts(dentry, target);
> +		mark_violated_mounts(target, dentry);
> +	}
> +
>  	dentry_lock_for_move(dentry, target);

> +void mnt_set_violated(struct dentry *root, struct dentry *moving)
> +{
> +	struct mountroot *mr;
> +	struct mount *mnt;
> +
> +	lock_mount_hash();
> +	mr = lookup_mountroot(root);
> +	if (!mr)
> +		goto out;
> +
> +	hlist_for_each_entry(mnt, &mr->r_list, mnt_mr_list) {
> +		struct mount *child;
> +		/* Be wary of this mount */
> +		mnt->mnt.mnt_flags |= MNT_VIOLATED;
> +
> +		list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
> +			/* Ignore children that will continue to be connected */
> +			if ((child->mnt_mountpoint != moving) &&
> +			    !d_ancestor(moving, child->mnt_mountpoint))
> +				continue;
> +
> +			/* Deal with mounts loosing the connection to
> +			 * their parents
> +			 */
> +			if (!(child->mnt.mnt_flags & MNT_UMOUNT)) {
> +				child->mnt.mnt_flags |= MNT_UNREACHABLE_PARENT;
> +				hlist_add_head(&child->mnt_pending_umount, &pending_umount);
> +				schedule_work(&pending_umount_work);
> +			} else {
> +				umount_mnt(child);
> +			}
> +		}
> +	}
> +out:
> +	unlock_mount_hash();

And that can have non-trivial security implications - ability to expose
something that has been overmounted is potentially very nasty.  I might
be missing something in the rest of the series (I'm half-asleep right now,
so that's certainly possible), but that doesn't look obviously safe.
Note that it's very different from the situation with umount-on-invalidation -
there the thing we are uncovering is dead.  In this one it might be very
much alive and deliberately covered...