[PATCH v2] devpts: Make ptmx be owned by the userns owner as a fallback

[PATCH v2] devpts: Make ptmx be owned by the userns owner as a fallback

[Andy Lutomirski]

New devpts instances have ptmx owned by the inner uid and gid 0.

For container-style namespaces (LXC, etc), this should have no
effect, this is fine.

For sandbox-style namespaces (xdg-app and similar), this is
problematic -- there may not be an inner 0:0.  If that happens,
devpts mounts will fail.

Fix it by adding a fallback: if 0:0 is not mapped but the userns
owner and group are mapped, then ptmx will be owned by the namespace
owner.

This won't change behavior except in cases where mount would
currently return -EINVAL.

Cc: Alexander Larsson <alexl@redhat.com>
Cc: mclasen@redhat.com
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Linux Containers <containers@lists.linux-foundation.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Changes from v1:
 - Reversed the preference order (Serge)
 - Fixed misuse of uid_valid on userns->owner

fs/devpts/inode.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 655f21f99160..42b1e04d8334 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -27,6 +27,7 @@
 #include <linux/parser.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
+#include <linux/user_namespace.h>
 
 #define DEVPTS_DEFAULT_MODE 0600
 /*
@@ -247,13 +248,33 @@ static int mknod_ptmx(struct super_block *sb)
 	struct dentry *root = sb->s_root;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
+	struct user_namespace *userns = current_user_ns();
 	kuid_t root_uid;
 	kgid_t root_gid;
 
-	root_uid = make_kuid(current_user_ns(), 0);
-	root_gid = make_kgid(current_user_ns(), 0);
-	if (!uid_valid(root_uid) || !gid_valid(root_gid))
-		return -EINVAL;
+	/*
+	 * For a new devpts instance, ptmx is owned by 0:0 if that uid
+	 * and gid are mapped in the creating namespace.
+	 */
+	root_uid = make_kuid(userns, 0);
+	root_gid = make_kgid(userns, 0);
+
+	if (!uid_valid(root_uid) || !gid_valid(root_gid)) {
+		/*
+		 * If the creating namespace does not have 0:0 mapped
+		 * but does have the owner mapped (this is rare in
+		 * container-style namespaces but common in
+		 * sandbox-style namespaces), then let ptmx be owned by
+		 * the namespace owner.
+		 */
+		root_uid = userns->owner;
+		root_gid = userns->group;
+
+		/* If this still doesn't work, give up. */
+		if (!kuid_has_mapping(userns, root_uid) ||
+		    !kgid_has_mapping(userns, root_gid))
+			return -EINVAL;
+	}
 
 	inode_lock(d_inode(root));
 
-- 
2.5.0

Re: [PATCH v2] devpts: Make ptmx be owned by the userns owner as a fallback

[Serge E. Hallyn]

Quoting Andy Lutomirski (luto@kernel.org):
> New devpts instances have ptmx owned by the inner uid and gid 0.
> 
> For container-style namespaces (LXC, etc), this should have no
> effect, this is fine.
> 
> For sandbox-style namespaces (xdg-app and similar), this is
> problematic -- there may not be an inner 0:0.  If that happens,
> devpts mounts will fail.
> 
> Fix it by adding a fallback: if 0:0 is not mapped but the userns
> owner and group are mapped, then ptmx will be owned by the namespace
> owner.
> 
> This won't change behavior except in cases where mount would
> currently return -EINVAL.
> 
> Cc: Alexander Larsson <alexl@redhat.com>
> Cc: mclasen@redhat.com
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Linux Containers <containers@lists.linux-foundation.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Thanks, looks good.

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> ---
> 
> Changes from v1:
>  - Reversed the preference order (Serge)
>  - Fixed misuse of uid_valid on userns->owner
> 
> fs/devpts/inode.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 655f21f99160..42b1e04d8334 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -27,6 +27,7 @@
>  #include <linux/parser.h>
>  #include <linux/fsnotify.h>
>  #include <linux/seq_file.h>
> +#include <linux/user_namespace.h>
>  
>  #define DEVPTS_DEFAULT_MODE 0600
>  /*
> @@ -247,13 +248,33 @@ static int mknod_ptmx(struct super_block *sb)
>  	struct dentry *root = sb->s_root;
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
>  	struct pts_mount_opts *opts = &fsi->mount_opts;
> +	struct user_namespace *userns = current_user_ns();
>  	kuid_t root_uid;
>  	kgid_t root_gid;
>  
> -	root_uid = make_kuid(current_user_ns(), 0);
> -	root_gid = make_kgid(current_user_ns(), 0);
> -	if (!uid_valid(root_uid) || !gid_valid(root_gid))
> -		return -EINVAL;
> +	/*
> +	 * For a new devpts instance, ptmx is owned by 0:0 if that uid
> +	 * and gid are mapped in the creating namespace.
> +	 */
> +	root_uid = make_kuid(userns, 0);
> +	root_gid = make_kgid(userns, 0);
> +
> +	if (!uid_valid(root_uid) || !gid_valid(root_gid)) {
> +		/*
> +		 * If the creating namespace does not have 0:0 mapped
> +		 * but does have the owner mapped (this is rare in
> +		 * container-style namespaces but common in
> +		 * sandbox-style namespaces), then let ptmx be owned by
> +		 * the namespace owner.
> +		 */
> +		root_uid = userns->owner;
> +		root_gid = userns->group;
> +
> +		/* If this still doesn't work, give up. */
> +		if (!kuid_has_mapping(userns, root_uid) ||
> +		    !kgid_has_mapping(userns, root_gid))
> +			return -EINVAL;
> +	}
>  
>  	inode_lock(d_inode(root));
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH v2] devpts: Make ptmx be owned by the userns owner as a fallback

[Andy Lutomirski]

On Tue, Mar 15, 2016 at 1:05 PM, Andy Lutomirski <luto@kernel.org> wrote:
> New devpts instances have ptmx owned by the inner uid and gid 0.
>
> For container-style namespaces (LXC, etc), this should have no
> effect, this is fine.
>
> For sandbox-style namespaces (xdg-app and similar), this is
> problematic -- there may not be an inner 0:0.  If that happens,
> devpts mounts will fail.
>
> Fix it by adding a fallback: if 0:0 is not mapped but the userns
> owner and group are mapped, then ptmx will be owned by the namespace
> owner.
>
> This won't change behavior except in cases where mount would
> currently return -EINVAL.

Eric, this patch is straightforward, fixes a real problem, and is
mostly orthogonal to the devpts stuff you're working on.  Could you
apply it for 4.7?

--Andy