diff mbox series

[04/43] separate copying and locking mount tree on cross-userns copies

Message ID 155059371731.12449.5751025556744658291.stgit@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show
Series VFS: Introduce filesystem context | expand

Commit Message

David Howells Feb. 19, 2019, 4:28 p.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

Rather than having propagate_mnt() check doing unprivileged copies,
lock them before commit_tree().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

 fs/namespace.c |   59 +++++++++++++++++++++++++++++++++++---------------------
 fs/pnode.c     |    5 -----
 fs/pnode.h     |    3 +--
 3 files changed, 38 insertions(+), 29 deletions(-)

Comments

Alan Jenkins Feb. 20, 2019, 6:55 p.m. UTC | #1 
On 19/02/2019 16:28, David Howells wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> Rather than having propagate_mnt() check doing unprivileged copies,
> lock them before commit_tree().
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>
>   fs/namespace.c |   59 +++++++++++++++++++++++++++++++++++---------------------
>   fs/pnode.c     |    5 -----
>   fs/pnode.h     |    3 +--
>   3 files changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index a677b59efd74..9ed2f2930dfd 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1013,27 +1013,6 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
>   
>   	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
>   	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL);
> -	/* Don't allow unprivileged users to change mount flags */
> -	if (flag & CL_UNPRIVILEGED) {
> -		mnt->mnt.mnt_flags |= MNT_LOCK_ATIME;
> -
> -		if (mnt->mnt.mnt_flags & MNT_READONLY)
> -			mnt->mnt.mnt_flags |= MNT_LOCK_READONLY;
> -
> -		if (mnt->mnt.mnt_flags & MNT_NODEV)
> -			mnt->mnt.mnt_flags |= MNT_LOCK_NODEV;
> -
> -		if (mnt->mnt.mnt_flags & MNT_NOSUID)
> -			mnt->mnt.mnt_flags |= MNT_LOCK_NOSUID;
> -
> -		if (mnt->mnt.mnt_flags & MNT_NOEXEC)
> -			mnt->mnt.mnt_flags |= MNT_LOCK_NOEXEC;
> -	}
> -
> -	/* Don't allow unprivileged users to reveal what is under a mount */
> -	if ((flag & CL_UNPRIVILEGED) &&
> -	    (!(flag & CL_EXPIRE) || list_empty(&old->mnt_expire)))
> -		mnt->mnt.mnt_flags |= MNT_LOCKED;
>   
>   	atomic_inc(&sb->s_active);
>   	mnt->mnt.mnt_sb = sb;
> @@ -1837,6 +1816,33 @@ int iterate_mounts(int (*f)(struct vfsmount *, void *), void *arg,
>   	return 0;
>   }
>   
> +static void lock_mnt_tree(struct mount *mnt)
> +{
> +	struct mount *p;
> +
> +	for (p = mnt; p; p = next_mnt(p, mnt)) {
> +		int flags = p->mnt.mnt_flags;
> +		/* Don't allow unprivileged users to change mount flags */
> +		flags |= MNT_LOCK_ATIME;
> +
> +		if (flags & MNT_READONLY)
> +			flags |= MNT_LOCK_READONLY;
> +
> +		if (flags & MNT_NODEV)
> +			flags |= MNT_LOCK_NODEV;
> +
> +		if (flags & MNT_NOSUID)
> +			flags |= MNT_LOCK_NOSUID;
> +
> +		if (flags & MNT_NOEXEC)
> +			flags |= MNT_LOCK_NOEXEC;
> +		/* Don't allow unprivileged users to reveal what is under a mount */
> +		if (list_empty(&p->mnt_expire))
> +			flags |= MNT_LOCKED;
> +		p->mnt.mnt_flags = flags;
> +	}
> +}
> +
>   static void cleanup_group_ids(struct mount *mnt, struct mount *end)
>   {
>   	struct mount *p;
> @@ -1954,6 +1960,7 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>   			struct mountpoint *dest_mp,
>   			struct path *parent_path)
>   {
> +	struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns;
>   	HLIST_HEAD(tree_list);
>   	struct mnt_namespace *ns = dest_mnt->mnt_ns;
>   	struct mountpoint *smp;
> @@ -2004,6 +2011,9 @@ static int attach_recursive_mnt(struct mount *source_mnt,
>   				 child->mnt_mountpoint);
>   		if (q)
>   			mnt_change_mountpoint(child, smp, q);
> +		/* Notice when we are propagating across user namespaces */
> +		if (child->mnt_parent->mnt_ns->user_ns != user_ns)
> +			lock_mnt_tree(child);
>   		commit_tree(child);
>   	}
>   	put_mountpoint(smp);
> @@ -2941,13 +2951,18 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
>   	/* First pass: copy the tree topology */
>   	copy_flags = CL_COPY_UNBINDABLE | CL_EXPIRE;
>   	if (user_ns != ns->user_ns)
> -		copy_flags |= CL_SHARED_TO_SLAVE | CL_UNPRIVILEGED;
> +		copy_flags |= CL_SHARED_TO_SLAVE;
>   	new = copy_tree(old, old->mnt.mnt_root, copy_flags);
>   	if (IS_ERR(new)) {
>   		namespace_unlock();
>   		free_mnt_ns(new_ns);
>   		return ERR_CAST(new);
>   	}
> +	if (user_ns != ns->user_ns) {
> +		lock_mount_hash();
> +		lock_mnt_tree(new);
> +		unlock_mount_hash();
> +	}
>   	new_ns->root = new;
>   	list_add_tail(&new_ns->list, &new->mnt_list);
>   
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 1100e810d855..7ea6cfb65077 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -214,7 +214,6 @@ static struct mount *next_group(struct mount *m, struct mount *origin)
>   }
>   
>   /* all accesses are serialized by namespace_sem */
> -static struct user_namespace *user_ns;
>   static struct mount *last_dest, *first_source, *last_source, *dest_master;
>   static struct mountpoint *mp;
>   static struct hlist_head *list;
> @@ -260,9 +259,6 @@ static int propagate_one(struct mount *m)
>   			type |= CL_MAKE_SHARED;
>   	}
>   		
> -	/* Notice when we are propagating across user namespaces */
> -	if (m->mnt_ns->user_ns != user_ns)
> -		type |= CL_UNPRIVILEGED;
>   	child = copy_tree(last_source, last_source->mnt.mnt_root, type);
>   	if (IS_ERR(child))
>   		return PTR_ERR(child);
> @@ -303,7 +299,6 @@ int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
>   	 * propagate_one(); everything is serialized by namespace_sem,
>   	 * so globals will do just fine.
>   	 */
> -	user_ns = current->nsproxy->mnt_ns->user_ns;
>   	last_dest = dest_mnt;
>   	first_source = source_mnt;
>   	last_source = source_mnt;
> diff --git a/fs/pnode.h b/fs/pnode.h
> index dc87e65becd2..3960a83666cf 100644
> --- a/fs/pnode.h
> +++ b/fs/pnode.h
> @@ -27,8 +27,7 @@
>   #define CL_MAKE_SHARED 		0x08
>   #define CL_PRIVATE 		0x10
>   #define CL_SHARED_TO_SLAVE	0x20
> -#define CL_UNPRIVILEGED		0x40
> -#define CL_COPY_MNT_NS_FILE	0x80
> +#define CL_COPY_MNT_NS_FILE	0x40
>   
>   #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
>   
>
>

I can see that this covers copy_mnt_ns().  It should also cover what 
will happen in future, if you pass an OPEN_TREE_CLONE fd to a process 
with a different mnt_ns and mnt_ns->user_ns, and that process mounts the 
fd using move_mount().  However, I can't work out how this covers mount 
propagation across namespaces.

The comment "Notice when we are propagating across user namespaces" is 
moved to attach_recursive_mnt().  I can't find any call to 
attach_recursive_mount() inside the mount propagation code.  Am I 
overlooking something?

Thanks
Alan
David Howells Feb. 26, 2019, 3:44 p.m. UTC | #2 
Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:

> I can see that this covers copy_mnt_ns().  It should also cover what will
> happen in future, if you pass an OPEN_TREE_CLONE fd to a process with a
> different mnt_ns and mnt_ns->user_ns, and that process mounts the fd using
> move_mount().  However, I can't work out how this covers mount propagation
> across namespaces.
> 
> The comment "Notice when we are propagating across user namespaces" is moved
> to attach_recursive_mnt().  I can't find any call to attach_recursive_mount()
> inside the mount propagation code.  Am I overlooking something?

You've spelt the function name two different ways?

Further, attach_recursive_mnt() calls propagation, not the other way round.

David (& Al)
Alan Jenkins Feb. 26, 2019, 5:45 p.m. UTC | #3 
On 26/02/2019 15:44, David Howells wrote:
> Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
>
>> I can see that this covers copy_mnt_ns().  It should also cover what will
>> happen in future, if you pass an OPEN_TREE_CLONE fd to a process with a
>> different mnt_ns and mnt_ns->user_ns, and that process mounts the fd using
>> move_mount().  However, I can't work out how this covers mount propagation
>> across namespaces.
>>
>> The comment "Notice when we are propagating across user namespaces" is moved
>> to attach_recursive_mnt().  I can't find any call to attach_recursive_mount()
>> inside the mount propagation code.  Am I overlooking something?
> You've spelt the function name two different ways?
>
> Further, attach_recursive_mnt() calls propagation, not the other way round.
>
> David (& Al)

Thanks!

I have a (positive) comment on the new mount API, that I was holding 
back due to my confusion here.  I will send it now.

Alan
diff mbox series

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index a677b59efd74..9ed2f2930dfd 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1013,27 +1013,6 @@  static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 
 	mnt->mnt.mnt_flags = old->mnt.mnt_flags;
 	mnt->mnt.mnt_flags &= ~(MNT_WRITE_HOLD|MNT_MARKED|MNT_INTERNAL);
-	/* Don't allow unprivileged users to change mount flags */
-	if (flag & CL_UNPRIVILEGED) {
-		mnt->mnt.mnt_flags |= MNT_LOCK_ATIME;
-
-		if (mnt->mnt.mnt_flags & MNT_READONLY)
-			mnt->mnt.mnt_flags |= MNT_LOCK_READONLY;
-
-		if (mnt->mnt.mnt_flags & MNT_NODEV)
-			mnt->mnt.mnt_flags |= MNT_LOCK_NODEV;
-
-		if (mnt->mnt.mnt_flags & MNT_NOSUID)
-			mnt->mnt.mnt_flags |= MNT_LOCK_NOSUID;
-
-		if (mnt->mnt.mnt_flags & MNT_NOEXEC)
-			mnt->mnt.mnt_flags |= MNT_LOCK_NOEXEC;
-	}
-
-	/* Don't allow unprivileged users to reveal what is under a mount */
-	if ((flag & CL_UNPRIVILEGED) &&
-	    (!(flag & CL_EXPIRE) || list_empty(&old->mnt_expire)))
-		mnt->mnt.mnt_flags |= MNT_LOCKED;
 
 	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_sb = sb;
@@ -1837,6 +1816,33 @@  int iterate_mounts(int (*f)(struct vfsmount *, void *), void *arg,
 	return 0;
 }
 
+static void lock_mnt_tree(struct mount *mnt)
+{
+	struct mount *p;
+
+	for (p = mnt; p; p = next_mnt(p, mnt)) {
+		int flags = p->mnt.mnt_flags;
+		/* Don't allow unprivileged users to change mount flags */
+		flags |= MNT_LOCK_ATIME;
+
+		if (flags & MNT_READONLY)
+			flags |= MNT_LOCK_READONLY;
+
+		if (flags & MNT_NODEV)
+			flags |= MNT_LOCK_NODEV;
+
+		if (flags & MNT_NOSUID)
+			flags |= MNT_LOCK_NOSUID;
+
+		if (flags & MNT_NOEXEC)
+			flags |= MNT_LOCK_NOEXEC;
+		/* Don't allow unprivileged users to reveal what is under a mount */
+		if (list_empty(&p->mnt_expire))
+			flags |= MNT_LOCKED;
+		p->mnt.mnt_flags = flags;
+	}
+}
+
 static void cleanup_group_ids(struct mount *mnt, struct mount *end)
 {
 	struct mount *p;
@@ -1954,6 +1960,7 @@  static int attach_recursive_mnt(struct mount *source_mnt,
 			struct mountpoint *dest_mp,
 			struct path *parent_path)
 {
+	struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns;
 	HLIST_HEAD(tree_list);
 	struct mnt_namespace *ns = dest_mnt->mnt_ns;
 	struct mountpoint *smp;
@@ -2004,6 +2011,9 @@  static int attach_recursive_mnt(struct mount *source_mnt,
 				 child->mnt_mountpoint);
 		if (q)
 			mnt_change_mountpoint(child, smp, q);
+		/* Notice when we are propagating across user namespaces */
+		if (child->mnt_parent->mnt_ns->user_ns != user_ns)
+			lock_mnt_tree(child);
 		commit_tree(child);
 	}
 	put_mountpoint(smp);
@@ -2941,13 +2951,18 @@  struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 	/* First pass: copy the tree topology */
 	copy_flags = CL_COPY_UNBINDABLE | CL_EXPIRE;
 	if (user_ns != ns->user_ns)
-		copy_flags |= CL_SHARED_TO_SLAVE | CL_UNPRIVILEGED;
+		copy_flags |= CL_SHARED_TO_SLAVE;
 	new = copy_tree(old, old->mnt.mnt_root, copy_flags);
 	if (IS_ERR(new)) {
 		namespace_unlock();
 		free_mnt_ns(new_ns);
 		return ERR_CAST(new);
 	}
+	if (user_ns != ns->user_ns) {
+		lock_mount_hash();
+		lock_mnt_tree(new);
+		unlock_mount_hash();
+	}
 	new_ns->root = new;
 	list_add_tail(&new_ns->list, &new->mnt_list);
 
diff --git a/fs/pnode.c b/fs/pnode.c
index 1100e810d855..7ea6cfb65077 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -214,7 +214,6 @@  static struct mount *next_group(struct mount *m, struct mount *origin)
 }
 
 /* all accesses are serialized by namespace_sem */
-static struct user_namespace *user_ns;
 static struct mount *last_dest, *first_source, *last_source, *dest_master;
 static struct mountpoint *mp;
 static struct hlist_head *list;
@@ -260,9 +259,6 @@  static int propagate_one(struct mount *m)
 			type |= CL_MAKE_SHARED;
 	}
 		
-	/* Notice when we are propagating across user namespaces */
-	if (m->mnt_ns->user_ns != user_ns)
-		type |= CL_UNPRIVILEGED;
 	child = copy_tree(last_source, last_source->mnt.mnt_root, type);
 	if (IS_ERR(child))
 		return PTR_ERR(child);
@@ -303,7 +299,6 @@  int propagate_mnt(struct mount *dest_mnt, struct mountpoint *dest_mp,
 	 * propagate_one(); everything is serialized by namespace_sem,
 	 * so globals will do just fine.
 	 */
-	user_ns = current->nsproxy->mnt_ns->user_ns;
 	last_dest = dest_mnt;
 	first_source = source_mnt;
 	last_source = source_mnt;
diff --git a/fs/pnode.h b/fs/pnode.h
index dc87e65becd2..3960a83666cf 100644
--- a/fs/pnode.h
+++ b/fs/pnode.h
@@ -27,8 +27,7 @@ 
 #define CL_MAKE_SHARED 		0x08
 #define CL_PRIVATE 		0x10
 #define CL_SHARED_TO_SLAVE	0x20
-#define CL_UNPRIVILEGED		0x40
-#define CL_COPY_MNT_NS_FILE	0x80
+#define CL_COPY_MNT_NS_FILE	0x40
 
 #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)