miscelaneous user namespace patches

When freeing a deeply nested user namespace free_user_ns calls put_user_ns on it's parent which may in turn call free_user_ns again. When -fno-optimize-sibling-calls is passed to gcc one stack frame per user namespace is left on the stack, potentially overflowing the kernel stack. CONFIG_FRAME_POINTER forces -fno-optimize-sibling-calls so we can't count on gcc to optimize this code.

Remove struct kref and use a plain atomic_t. Making the code more flexible and easier to comprehend. Make the loop in free_user_ns explict to guarantee that the stack does not overflow with CONFIG_FRAME_POINTER enabled.

I have tested this fix with a simple program that uses unshare to create a deeply nested user namespace structure and then calls exit. With 1000 nesteuser namespaces before this change running my test program causes the kernel to die a horrible death. With 10,000,000 nested user namespaces after this change my test program runs to completion and causes no harm.

Pointed-out-by Vasily Kulikov <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 include/linux/user_namespace.h |   10 +++++-----
 kernel/user.c                  |    4 +---
 kernel/user_namespace.c        |   17 +++++++++--------
 3 files changed, 15 insertions(+), 16 deletions(-)

include/linux/user_namespace.h +5 -5

index b9bd2e6..4ce0093 100644
@@ -21,7 +21,7 @@ struct user_namespace {
 	struct uid_gid_map	uid_map;
 	struct uid_gid_map	gid_map;
 	struct uid_gid_map	projid_map;
-	struct kref		kref;
+	atomic_t		count;
 	struct user_namespace	*parent;
 	kuid_t			owner;
 	kgid_t			group;

@@ -35,18 +35,18 @@ extern struct user_namespace init_user_ns;
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
 {
 	if (ns)
-		kref_get(&ns->kref);
+		atomic_inc(&ns->count);
 	return ns;
 }
 
 extern int create_user_ns(struct cred *new);
 extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred);
-extern void free_user_ns(struct kref *kref);
+extern void free_user_ns(struct user_namespace *ns);
 
 static inline void put_user_ns(struct user_namespace *ns)
 {
-	if (ns)
-		kref_put(&ns->kref, free_user_ns);
+	if (ns && atomic_dec_and_test(&ns->count))
+		free_user_ns(ns);
 }
 
 struct seq_operations;

kernel/user.c +1 -3

index 33acb5e..57ebfd4 100644
@@ -47,9 +47,7 @@ struct user_namespace init_user_ns = {
 			.count = 4294967295U,
 		},
 	},
-	.kref = {
-		.refcount	= ATOMIC_INIT(3),
-	},
+	.count = ATOMIC_INIT(3),
 	.owner = GLOBAL_ROOT_UID,
 	.group = GLOBAL_ROOT_GID,
 	.proc_inum = PROC_USER_INIT_INO,

kernel/user_namespace.c +9 -8

index 2b042c4..24f8ec3 100644
@@ -78,7 +78,7 @@ int create_user_ns(struct cred *new)
 		return ret;
 	}
 
-	kref_init(&ns->kref);
+	atomic_set(&ns->count, 1);
 	/* Leave the new->user_ns reference with the new user namespace. */
 	ns->parent = parent_ns;
 	ns->owner = owner;

@@ -104,15 +104,16 @@ int unshare_userns(unsigned long unshare_flags, struct cred **new_cred)
 	return create_user_ns(cred);
 }
 
-void free_user_ns(struct kref *kref)
+void free_user_ns(struct user_namespace *ns)
 {
-	struct user_namespace *parent, *ns =
-		container_of(kref, struct user_namespace, kref);
+	struct user_namespace *parent;
 
-	parent = ns->parent;
-	proc_free_inum(ns->proc_inum);
-	kmem_cache_free(user_ns_cachep, ns);
-	put_user_ns(parent);
+	do {
+		parent = ns->parent;
+		proc_free_inum(ns->proc_inum);
+		kmem_cache_free(user_ns_cachep, ns);
+		ns = parent;
+	} while (atomic_dec_and_test(&parent->count));
 }
 EXPORT_SYMBOL(free_user_ns);
 

Signature · 1 line

1.7.5.4

When freeing a deeply nested user namespace free_user_ns calls put_user_ns on it's parent which may in turn call free_user_ns again. When -fno-optimize-sibling-calls is passed to gcc one stack frame per user namespace is left on the stack, potentially overflowing the kernel stack. CONFIG_FRAME_POINTER forces -fno-optimize-sibling-calls so we can't count on gcc to optimize this code.

Remove struct kref and use a plain atomic_t. Making the code more flexible and easier to comprehend. Make the loop in free_user_ns explict to guarantee that the stack does not overflow with CONFIG_FRAME_POINTER enabled.

I have tested this fix with a simple program that uses unshare to create a deeply nested user namespace structure and then calls exit. With 1000 nesteuser namespaces before this change running my test program causes the kernel to die a horrible death. With 10,000,000 nested user namespaces after this change my test program runs to completion and causes no harm.

Pointed-out-by Vasily Kulikov <segoon-cxoSlKxDwOJWk0Htik3J/[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 include/linux/user_namespace.h |   10 +++++-----
 kernel/user.c                  |    4 +---
 kernel/user_namespace.c        |   17 +++++++++--------
 3 files changed, 15 insertions(+), 16 deletions(-)

include/linux/user_namespace.h +5 -5

index b9bd2e6..4ce0093 100644
@@ -21,7 +21,7 @@ struct user_namespace {
 	struct uid_gid_map	uid_map;
 	struct uid_gid_map	gid_map;
 	struct uid_gid_map	projid_map;
-	struct kref		kref;
+	atomic_t		count;
 	struct user_namespace	*parent;
 	kuid_t			owner;
 	kgid_t			group;

@@ -35,18 +35,18 @@ extern struct user_namespace init_user_ns;
 static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
 {
 	if (ns)
-		kref_get(&ns->kref);
+		atomic_inc(&ns->count);
 	return ns;
 }
 
 extern int create_user_ns(struct cred *new);
 extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred);
-extern void free_user_ns(struct kref *kref);
+extern void free_user_ns(struct user_namespace *ns);
 
 static inline void put_user_ns(struct user_namespace *ns)
 {
-	if (ns)
-		kref_put(&ns->kref, free_user_ns);
+	if (ns && atomic_dec_and_test(&ns->count))
+		free_user_ns(ns);
 }
 
 struct seq_operations;

kernel/user.c +1 -3

index 33acb5e..57ebfd4 100644
@@ -47,9 +47,7 @@ struct user_namespace init_user_ns = {
 			.count = 4294967295U,
 		},
 	},
-	.kref = {
-		.refcount	= ATOMIC_INIT(3),
-	},
+	.count = ATOMIC_INIT(3),
 	.owner = GLOBAL_ROOT_UID,
 	.group = GLOBAL_ROOT_GID,
 	.proc_inum = PROC_USER_INIT_INO,

kernel/user_namespace.c +9 -8

index 2b042c4..24f8ec3 100644
@@ -78,7 +78,7 @@ int create_user_ns(struct cred *new)
 		return ret;
 	}
 
-	kref_init(&ns->kref);
+	atomic_set(&ns->count, 1);
 	/* Leave the new->user_ns reference with the new user namespace. */
 	ns->parent = parent_ns;
 	ns->owner = owner;

@@ -104,15 +104,16 @@ int unshare_userns(unsigned long unshare_flags, struct cred **new_cred)
 	return create_user_ns(cred);
 }
 
-void free_user_ns(struct kref *kref)
+void free_user_ns(struct user_namespace *ns)
 {
-	struct user_namespace *parent, *ns =
-		container_of(kref, struct user_namespace, kref);
+	struct user_namespace *parent;
 
-	parent = ns->parent;
-	proc_free_inum(ns->proc_inum);
-	kmem_cache_free(user_ns_cachep, ns);
-	put_user_ns(parent);
+	do {
+		parent = ns->parent;
+		proc_free_inum(ns->proc_inum);
+		kmem_cache_free(user_ns_cachep, ns);
+		ns = parent;
+	} while (atomic_dec_and_test(&parent->count));
 }
 EXPORT_SYMBOL(free_user_ns);
 

Signature · 1 line

1.7.5.4

When freeing a deeply nested user namespace free_user_ns calls put_user_ns on it's parent which may in turn call free_user_ns again. When -fno-optimize-sibling-calls is passed to gcc one stack frame per user namespace is left on the stack, potentially overflowing the kernel stack. CONFIG_FRAME_POINTER forces -fno-optimize-sibling-calls so we can't count on gcc to optimize this code.

Remove struct kref and use a plain atomic_t. Making the code more flexible and easier to comprehend. Make the loop in free_user_ns explict to guarantee that the stack does not overflow with CONFIG_FRAME_POINTER enabled.

I have tested this fix with a simple program that uses unshare to create a deeply nested user namespace structure and then calls exit. With 1000 nesteuser namespaces before this change running my test program causes the kernel to die a horrible death. With 10,000,000 nested user namespaces after this change my test program runs to completion and causes no harm.

Pointed-out-by Vasily Kulikov <segoon-cxoSlKxDwOJWk0Htik3J/[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

Acked-by Vasily Kulikov <segoon-cxoSlKxDwOJWk0Htik3J/[email protected]>

The second bug I've noted in the same email (OOM) looks like should be "fixed" by using memcg to limit kernel memory. So, I'm fine with this side of user_ns :)

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/user_namespace.c |   45 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 39 insertions(+), 6 deletions(-)

kernel/user_namespace.c +39 -6

index 24f8ec3..8b65083 100644
@@ -520,6 +520,42 @@ struct seq_operations proc_projid_seq_operations = {
 	.show = projid_m_show,
 };
 
+static bool mappings_overlap(struct uid_gid_map *new_map, struct uid_gid_extent *extent)
+{
+	u32 upper_first, lower_first, upper_last, lower_last;
+	unsigned idx;
+
+	upper_first = extent->first;
+	lower_first = extent->lower_first;
+	upper_last = upper_first + extent->count - 1;
+	lower_last = lower_first + extent->count - 1;
+
+	for (idx = 0; idx < new_map->nr_extents; idx++) {
+		u32 prev_upper_first, prev_lower_first;
+		u32 prev_upper_last, prev_lower_last;
+		struct uid_gid_extent *prev;
+
+		prev = &new_map->extent[idx];
+
+		prev_upper_first = prev->first;
+		prev_lower_first = prev->lower_first;
+		prev_upper_last = prev_upper_first + prev->count - 1;
+		prev_lower_last = prev_lower_first + prev->count - 1;
+
+		/* Does the upper range intersect a previous extent? */
+		if ((prev_upper_first <= upper_last) &&
+		    (prev_upper_last >= upper_first))
+			return true;
+
+		/* Does the lower range intersect a previous extent? */
+		if ((prev_lower_first <= lower_last) &&
+		    (prev_lower_last >= lower_first))
+			return true;
+	}
+	return false;
+}
+
+
 static DEFINE_MUTEX(id_map_mutex);
 
 static ssize_t map_write(struct file *file, const char __user *buf,

@@ -532,7 +568,7 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	struct user_namespace *ns = seq->private;
 	struct uid_gid_map new_map;
 	unsigned idx;
-	struct uid_gid_extent *extent, *last = NULL;
+	struct uid_gid_extent *extent = NULL;
 	unsigned long page = 0;
 	char *kbuf, *pos, *next_line;
 	ssize_t ret = -EINVAL;

@@ -635,14 +671,11 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 		if ((extent->lower_first + extent->count) <= extent->lower_first)
 			goto out;
 
-		/* For now only accept extents that are strictly in order */
-		if (last &&
-		    (((last->first + last->count) > extent->first) ||
-		     ((last->lower_first + last->count) > extent->lower_first)))
+		/* Do the ranges in extent overlap any previous extents? */
+		if (mappings_overlap(&new_map, extent))
 			goto out;
 
 		new_map.nr_extents++;
-		last = extent;
 
 		/* Fail if the file contains too many extents */
 		if ((new_map.nr_extents == UID_GID_MAP_MAX_EXTENTS) &&

Signature · 1 line

1.7.5.4

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 kernel/user_namespace.c |   45 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 39 insertions(+), 6 deletions(-)

kernel/user_namespace.c +39 -6

index 24f8ec3..8b65083 100644
@@ -520,6 +520,42 @@ struct seq_operations proc_projid_seq_operations = {
 	.show = projid_m_show,
 };
 
+static bool mappings_overlap(struct uid_gid_map *new_map, struct uid_gid_extent *extent)
+{
+	u32 upper_first, lower_first, upper_last, lower_last;
+	unsigned idx;
+
+	upper_first = extent->first;
+	lower_first = extent->lower_first;
+	upper_last = upper_first + extent->count - 1;
+	lower_last = lower_first + extent->count - 1;
+
+	for (idx = 0; idx < new_map->nr_extents; idx++) {
+		u32 prev_upper_first, prev_lower_first;
+		u32 prev_upper_last, prev_lower_last;
+		struct uid_gid_extent *prev;
+
+		prev = &new_map->extent[idx];
+
+		prev_upper_first = prev->first;
+		prev_lower_first = prev->lower_first;
+		prev_upper_last = prev_upper_first + prev->count - 1;
+		prev_lower_last = prev_lower_first + prev->count - 1;
+
+		/* Does the upper range intersect a previous extent? */
+		if ((prev_upper_first <= upper_last) &&
+		    (prev_upper_last >= upper_first))
+			return true;
+
+		/* Does the lower range intersect a previous extent? */
+		if ((prev_lower_first <= lower_last) &&
+		    (prev_lower_last >= lower_first))
+			return true;
+	}
+	return false;
+}
+
+
 static DEFINE_MUTEX(id_map_mutex);
 
 static ssize_t map_write(struct file *file, const char __user *buf,

@@ -532,7 +568,7 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	struct user_namespace *ns = seq->private;
 	struct uid_gid_map new_map;
 	unsigned idx;
-	struct uid_gid_extent *extent, *last = NULL;
+	struct uid_gid_extent *extent = NULL;
 	unsigned long page = 0;
 	char *kbuf, *pos, *next_line;
 	ssize_t ret = -EINVAL;

@@ -635,14 +671,11 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 		if ((extent->lower_first + extent->count) <= extent->lower_first)
 			goto out;
 
-		/* For now only accept extents that are strictly in order */
-		if (last &&
-		    (((last->first + last->count) > extent->first) ||
-		     ((last->lower_first + last->count) > extent->lower_first)))
+		/* Do the ranges in extent overlap any previous extents? */
+		if (mappings_overlap(&new_map, extent))
 			goto out;
 
 		new_map.nr_extents++;
-		last = extent;
 
 		/* Fail if the file contains too many extents */
 		if ((new_map.nr_extents == UID_GID_MAP_MAX_EXTENTS) &&

Signature · 1 line

1.7.5.4

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/user_namespace.c |   45 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 39 insertions(+), 6 deletions(-)

kernel/user_namespace.c +39 -6

index 24f8ec3..8b65083 100644
@@ -520,6 +520,42 @@ struct seq_operations proc_projid_seq_operations = {
 	.show = projid_m_show,
 };
 
+static bool mappings_overlap(struct uid_gid_map *new_map, struct uid_gid_extent *extent)
+{
+	u32 upper_first, lower_first, upper_last, lower_last;
+	unsigned idx;
+
+	upper_first = extent->first;
+	lower_first = extent->lower_first;
+	upper_last = upper_first + extent->count - 1;
+	lower_last = lower_first + extent->count - 1;
+
+	for (idx = 0; idx < new_map->nr_extents; idx++) {
+		u32 prev_upper_first, prev_lower_first;
+		u32 prev_upper_last, prev_lower_last;
+		struct uid_gid_extent *prev;
+
+		prev = &new_map->extent[idx];
+
+		prev_upper_first = prev->first;
+		prev_lower_first = prev->lower_first;
+		prev_upper_last = prev_upper_first + prev->count - 1;
+		prev_lower_last = prev_lower_first + prev->count - 1;
+
+		/* Does the upper range intersect a previous extent? */
+		if ((prev_upper_first <= upper_last) &&
+		    (prev_upper_last >= upper_first))
+			return true;
+
+		/* Does the lower range intersect a previous extent? */
+		if ((prev_lower_first <= lower_last) &&
+		    (prev_lower_last >= lower_first))
+			return true;
+	}
+	return false;
+}
+
+
 static DEFINE_MUTEX(id_map_mutex);
 
 static ssize_t map_write(struct file *file, const char __user *buf,

@@ -532,7 +568,7 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	struct user_namespace *ns = seq->private;
 	struct uid_gid_map new_map;
 	unsigned idx;
-	struct uid_gid_extent *extent, *last = NULL;
+	struct uid_gid_extent *extent = NULL;
 	unsigned long page = 0;
 	char *kbuf, *pos, *next_line;
 	ssize_t ret = -EINVAL;

@@ -635,14 +671,11 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 		if ((extent->lower_first + extent->count) <= extent->lower_first)
 			goto out;
 
-		/* For now only accept extents that are strictly in order */
-		if (last &&
-		    (((last->first + last->count) > extent->first) ||
-		     ((last->lower_first + last->count) > extent->lower_first)))
+		/* Do the ranges in extent overlap any previous extents? */
+		if (mappings_overlap(&new_map, extent))
 			goto out;
 
 		new_map.nr_extents++;
-		last = extent;
 
 		/* Fail if the file contains too many extents */
 		if ((new_map.nr_extents == UID_GID_MAP_MAX_EXTENTS) &&

Signature · 1 line

1.7.5.4

Quoted 5 lines · Eric W. Biederman

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Quoted 3 lines

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Acked-by Someone <[email protected]>

Hello Mr. Someone.

Quoted 6 lines · Aristeu Rozanski

Quoted 5 lines · Eric W. Biederman

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Quoted 6 lines

Quoted 3 lines

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Acked-by Someone <[email protected]>

Documentation/SubmittingPatches:

"then you just add a line saying

Signed-off-by Random J Developer <[email protected]>

using your real name (sorry, no pseudonyms or anonymous contributions.)"

I know how it feels, but that is how it goes. You'll have to change that.

Quoted 5 lines · Eric W. Biederman

When I initially wrote the code for /proc/<pid>/uid_map. I was lazy and avoided duplicate mappings by the simple expedient of ensuring the first number in a new extent was greater than any number in the previous extent.

Quoted 3 lines

Unfortunately that precludes a number of valid mappings, and someone noticed and complained. So use a simple check to ensure that ranges in the mapping extents don't overlap.

Acked-by Someone <[email protected]>

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 Documentation/namespaces/resource-control.txt |   10 ++++++++++
 init/Kconfig                                  |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/namespaces/resource-control.txt

Documentation/namespaces/resource-control.txt new file +0 -0

new file mode 100644
index 0000000..3d8178a

Documentation/namespaces/resource-control.txt new file +5 -0

+
+Therefore it is recommended that memory control groups be enabled in
+kernels that enable user namespaces, and it is further recommended
+that userspace configure memory control groups to limit how much
+memory users they don't trust to play nice can use.

init/Kconfig +7 -0

index 7d30240..c8c58bd 100644
@@ -1035,6 +1035,13 @@ config USER_NS
 	help
 	  This allows containers, i.e. vservers, to use user namespaces
 	  to provide different user info for different servers.
+
+	  When user namespaces are enabled in the kernel it is
+	  recommended that the MEMCG and MEMCG_KMEM options also be
+	  enabled and that user-space use the memory control groups to
+	  limit the amount of memory a memory unprivileged users can
+	  use.
+
 	  If unsure, say N.
 
 config PID_NS

Signature · 1 line

1.7.5.4

Quoted 6 lines · Eric W. Biederman

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Quoted 1 line

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

Acked-by Serge Hallyn <[email protected]>

nit:

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 Documentation/namespaces/resource-control.txt |   10 ++++++++++
 init/Kconfig                                  |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/namespaces/resource-control.txt

Documentation/namespaces/resource-control.txt new file +10 -0

new file mode 100644
index 0000000..3d8178a
@@ -0,0 +1,10 @@
+There are a lot of kinds of objects in the kernel that don't have
+individual limits or that have limits that are ineffective when a set
+of processes is allowed to switch user ids.  With user namespaces
+enabled in a kernel for people who don't trust their users or their
+users programs to play nice this problems becomes more acute.
+
+Therefore it is recommended that memory control groups be enabled in
+kernels that enable user namespaces, and it is further recommended
+that userspace configure memory control groups to limit how much
+memory users they don't trust to play nice can use.

init/Kconfig +0 -0

index 7d30240..c8c58bd 100644
@@ -1035,6 +1035,13 @@ config USER_NS
 	help

Quoted 35 lines · Eric W. Biederman

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 Documentation/namespaces/resource-control.txt |   10 ++++++++++
 init/Kconfig                                  |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/namespaces/resource-control.txt

Documentation/namespaces/resource-control.txt new file +10 -0

new file mode 100644
index 0000000..3d8178a
@@ -0,0 +1,10 @@
+There are a lot of kinds of objects in the kernel that don't have
+individual limits or that have limits that are ineffective when a set
+of processes is allowed to switch user ids.  With user namespaces
+enabled in a kernel for people who don't trust their users or their
+users programs to play nice this problems becomes more acute.
+
+Therefore it is recommended that memory control groups be enabled in
+kernels that enable user namespaces, and it is further recommended
+that userspace configure memory control groups to limit how much
+memory users they don't trust to play nice can use.

init/Kconfig +0 -0

index 7d30240..c8c58bd 100644
@@ -1035,6 +1035,13 @@ config USER_NS
 	help

init/Kconfig config USER_NS · line 1045

Quoted 10 lines

init/Kconfig +7 -0

 	  This allows containers, i.e. vservers, to use user namespaces
 	  to provide different user info for different servers.
+
+	  When user namespaces are enabled in the kernel it is
+	  recommended that the MEMCG and MEMCG_KMEM options also be
+	  enabled and that user-space use the memory control groups to
+	  limit the amount of memory a memory unprivileged users can
+	  use.
+
 	  If unsure, say N.

Since this becomes an official recommendation that people will likely follow, are we really that much concerned about the types of abuses the MEMCG_KMEM will prevent? Those are mostly metadata-based abuses users could do in their own local disks without mounting anything extra (and things that look like that)

Unless there is a specific concern here, shouldn't we say "... that the MEMCG (and possibly MEMCG_KMEM) options..." ?

I just saw in a later patch of yours that your concern here seems not limited to backed ram by tmpfs, but with things like the internal structures for userns , to avoid patterns in the form: 'for (;;) unshare(...)'

Humm, it does seem sensible. The kernel memory controller aims to prevent exactly things like that. But they all exist already before
userns: there are destructive patterns like that with sockets, dentries, processes, and pretty much every other resource in the kernel. So Although the recommendation per-se makes sense, I am wondering if it is worth it to mention anything in the user_ns config?

I just saw in a later patch of yours that your concern here seems not limited to backed ram by tmpfs, but with things like the internal structures for userns , to avoid patterns in the form: 'for (;;) unshare(...)'

Humm, it does seem sensible. The kernel memory controller aims to prevent exactly things like that. But they all exist already before
userns: there are destructive patterns like that with sockets, dentries, processes, and pretty much every other resource in the kernel. So Although the recommendation per-se makes sense, I am wondering if it is worth it to mention anything in the user_ns config?

Quoted 11 lines · Lord Glauber Costa of Sealand

I just saw in a later patch of yours that your concern here seems not limited to backed ram by tmpfs, but with things like the internal structures for userns , to avoid patterns in the form: 'for (;;) unshare(...)'

Humm, it does seem sensible. The kernel memory controller aims to prevent exactly things like that. But they all exist already before
userns: there are destructive patterns like that with sockets, dentries, processes, and pretty much every other resource in the kernel. So Although the recommendation per-se makes sense, I am wondering if it is worth it to mention anything in the user_ns config?

The config might be overkill. However I have already gotten bug reports about there being no limits.

So someone needs to stop and connect the dots and say:

"If you care this is what you can do."

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Lord Glauber Costa of Sealand

I just saw in a later patch of yours that your concern here seems not limited to backed ram by tmpfs, but with things like the internal structures for userns , to avoid patterns in the form: 'for (;;) unshare(...)'

Humm, it does seem sensible. The kernel memory controller aims to prevent exactly things like that. But they all exist already before
userns: there are destructive patterns like that with sockets, dentries, processes, and pretty much every other resource in the kernel. So Although the recommendation per-se makes sense, I am wondering if it is worth it to mention anything in the user_ns config?

Quoted 4 lines

The config might be overkill. However I have already gotten bug reports about there being no limits.

So someone needs to stop and connect the dots and say:

Absolutely, and I am all for it

Quoted 1 line

"If you care this is what you can do."

How about we say it, then?

The current text in quite cryptic in this aspect, in the sense that it doesn't give enough information for standard people about what are the problems involved.

Of course, maybe the Kconfig text is not the best place for having all the info: but don't we have some place in Documentation/ where we could put this, and then refer people there from Kconfig ?

Quoted 29 lines · Lord Glauber Costa of Sealand

Quoted 14 lines · Eric W. Biederman

Quoted 11 lines · Lord Glauber Costa of Sealand

I just saw in a later patch of yours that your concern here seems not limited to backed ram by tmpfs, but with things like the internal structures for userns , to avoid patterns in the form: 'for (;;) unshare(...)'

Humm, it does seem sensible. The kernel memory controller aims to prevent exactly things like that. But they all exist already before
userns: there are destructive patterns like that with sockets, dentries, processes, and pretty much every other resource in the kernel. So Although the recommendation per-se makes sense, I am wondering if it is worth it to mention anything in the user_ns config?

Quoted 4 lines

The config might be overkill. However I have already gotten bug reports about there being no limits.

So someone needs to stop and connect the dots and say:

Absolutely, and I am all for it

Quoted 1 line

"If you care this is what you can do."

How about we say it, then?

The current text in quite cryptic in this aspect, in the sense that it doesn't give enough information for standard people about what are the problems involved.

Quoted 3 lines

Of course, maybe the Kconfig text is not the best place for having all the info: but don't we have some place in Documentation/ where we could put this, and then refer people there from Kconfig ?

At this point I have written the best text I can.

Please feel free to look at my tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next

and send me an patch on top of that to improve the wording.

At this point I have done my best to connect the dots for people who care, that the memory control group is what they need to limit what people can do with user namespaces.

My hope is that there is at least a passing mention in the next user namespace article on lwn.

For two pieces of software that were designed to complement each other I find it a bit surprising how many people (including myself) need the connection made that memory control groups and user namespaces should go together.

Quoted 4 lines

For two pieces of software that were designed to complement each other I find it a bit surprising how many people (including myself) need the connection made that memory control groups and user namespaces should go together.

Well, I've manifested many times in here that I am less than satisfied about the fact that the connection between namespaces and cgroups are so loose. There are many situations, like virtualizing the proc files and friends where I believe we could benefit from having the information about whether or not cgroups and namespaces are used at the same time.

Quoted 4 lines

For two pieces of software that were designed to complement each other I find it a bit surprising how many people (including myself) need the connection made that memory control groups and user namespaces should go together.

Well, I've manifested many times in here that I am less than satisfied about the fact that the connection between namespaces and cgroups are so loose. There are many situations, like virtualizing the proc files and friends where I believe we could benefit from having the information about whether or not cgroups and namespaces are used at the same time.

Quoted 35 lines · Eric W. Biederman

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 Documentation/namespaces/resource-control.txt |   10 ++++++++++
 init/Kconfig                                  |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/namespaces/resource-control.txt

Documentation/namespaces/resource-control.txt new file +10 -0

new file mode 100644
index 0000000..3d8178a
@@ -0,0 +1,10 @@
+There are a lot of kinds of objects in the kernel that don't have
+individual limits or that have limits that are ineffective when a set
+of processes is allowed to switch user ids.  With user namespaces
+enabled in a kernel for people who don't trust their users or their
+users programs to play nice this problems becomes more acute.
+
+Therefore it is recommended that memory control groups be enabled in
+kernels that enable user namespaces, and it is further recommended
+that userspace configure memory control groups to limit how much
+memory users they don't trust to play nice can use.

init/Kconfig +0 -0

index 7d30240..c8c58bd 100644
@@ -1035,6 +1035,13 @@ config USER_NS
 	help

init/Kconfig config USER_NS · line 1045

Quoted 10 lines

init/Kconfig +7 -0

 	  This allows containers, i.e. vservers, to use user namespaces
 	  to provide different user info for different servers.
+
+	  When user namespaces are enabled in the kernel it is
+	  recommended that the MEMCG and MEMCG_KMEM options also be
+	  enabled and that user-space use the memory control groups to
+	  limit the amount of memory a memory unprivileged users can
+	  use.
+
 	  If unsure, say N.

Since this becomes an official recommendation that people will likely follow, are we really that much concerned about the types of abuses the MEMCG_KMEM will prevent? Those are mostly metadata-based abuses users could do in their own local disks without mounting anything extra (and things that look like that)

Unless there is a specific concern here, shouldn't we say "... that the MEMCG (and possibly MEMCG_KMEM) options..." ?

In the help text describing user namespaces recommend use of memory control groups. In many cases memory control groups are the only mechanism there is to limit how much memory a user who can create user namespaces can use.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 Documentation/namespaces/resource-control.txt |   10 ++++++++++
 init/Kconfig                                  |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/namespaces/resource-control.txt

Documentation/namespaces/resource-control.txt new file +10 -0

new file mode 100644
index 0000000..3d8178a
@@ -0,0 +1,10 @@
+There are a lot of kinds of objects in the kernel that don't have
+individual limits or that have limits that are ineffective when a set
+of processes is allowed to switch user ids.  With user namespaces
+enabled in a kernel for people who don't trust their users or their
+users programs to play nice this problems becomes more acute.
+
+Therefore it is recommended that memory control groups be enabled in
+kernels that enable user namespaces, and it is further recommended
+that userspace configure memory control groups to limit how much
+memory users they don't trust to play nice can use.

init/Kconfig +0 -0

index 7d30240..c8c58bd 100644
@@ -1035,6 +1035,13 @@ config USER_NS
 	help

- The context in which devpts is mounted has no effect on the creation of ptys as the /dev/ptmx interface has been used by unprivileged users for many years.

- Only support unprivileged mounts in combination with the newinstance option to ensure that mounting of /dev/pts in a user namespace will not allow the options of an existing mount of devpts to be modified.

- Create /dev/pts/ptmx as the root user in the user namespace that mounts devpts so that it's permissions to be changed.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/devpts/inode.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

fs/devpts/inode.c +18 -0

index 472e6be..073d30b 100644
@@ -243,6 +243,13 @@ 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;
+	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;
 
 	mutex_lock(&root->d_inode->i_mutex);
 

@@ -273,6 +280,8 @@ static int mknod_ptmx(struct super_block *sb)
 
 	mode = S_IFCHR|opts->ptmxmode;
 	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
+	inode->i_uid = root_uid;
+	inode->i_gid = root_gid;
 
 	d_add(dentry, inode);
 

@@ -438,6 +447,12 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	if (error)
 		return ERR_PTR(error);
 
+	/* Require newinstance for all user namespace mounts to ensure
+	 * the mount options are not changed.
+	 */
+	if ((current_user_ns() != &init_user_ns) && !opts.newinstance)
+		return ERR_PTR(-EINVAL);
+
 	if (opts.newinstance)
 		s = sget(fs_type, NULL, set_anon_super, flags, NULL);
 	else

@@ -491,6 +506,9 @@ static struct file_system_type devpts_fs_type = {
 	.name		= "devpts",
 	.mount		= devpts_mount,
 	.kill_sb	= devpts_kill_sb,
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+	.fs_flags	= FS_USERNS_MOUNT | FS_USERNS_DEV_MOUNT,
+#endif
 };
 
 /*

Signature · 1 line

1.7.5.4

There is no backing store to ramfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it.

The memory control group successfully limits how much memory ramfs can consume on any system that cares about a user namespace root using ramfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/ramfs/inode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/ramfs/inode.c +1 -0

index eab8c09..c24f1e1 100644
@@ -260,6 +260,7 @@ static struct file_system_type ramfs_fs_type = {
 	.name		= "ramfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= ramfs_kill_sb,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 static struct file_system_type rootfs_fs_type = {
 	.name		= "rootfs",

Signature · 1 line

1.7.5.4

Quoted 10 lines · Eric W. Biederman

There is no backing store to ramfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it.

The memory control group successfully limits how much memory ramfs can consume on any system that cares about a user namespace root using ramfs to exhaust memory the memory control group can be deployed.

But that does mean that to avoid this new type of attack, when handed a new kernel (i.e. by one's distro) one has to explicitly (know about and) configure those limits. The "your distro should do this for you" argument doesn't seem right. And I'd really prefer there not be barriers to user namespaces being compiled in when there don't have to be.

What was your thought on the suggestion to only allow FS_USERNS_MOUNT mounts by users confined in a non-init memory cgroup?

Alternatively, what about a simple sysctl knob to turn on FS_USERNS_MOUNTs? Then if I've got no untrusted users I can just turn that on without the system second-guessing me for not having extra configuration...

Quoted 12 lines · Serge E. Hallyn

Quoted 10 lines · Eric W. Biederman

There is no backing store to ramfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it.

The memory control group successfully limits how much memory ramfs can consume on any system that cares about a user namespace root using ramfs to exhaust memory the memory control group can be deployed.

Quoted 6 lines

But that does mean that to avoid this new type of attack, when handed a new kernel (i.e. by one's distro) one has to explicitly (know about and) configure those limits. The "your distro should do this for you" argument doesn't seem right. And I'd really prefer there not be barriers to user namespaces being compiled in when there don't have to be.

The thing is this really isn't a new type of attack. There are a lot of

existing methods to exhaust memory with the default configuration on most distros. All this is is a new method to method to implement such an attack.

Quoted 2 lines

What was your thought on the suggestion to only allow FS_USERNS_MOUNT mounts by users confined in a non-init memory cgroup?

Over design.

So shrug. The mechanisms that I am suggesting people use already exist, and appear to have been present long enough to have made it into debian stable release February of 2011.

There is no backing store to ramfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it.

The memory control group successfully limits how much memory ramfs can consume on any system that cares about a user namespace root using ramfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/ramfs/inode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/ramfs/inode.c +1 -0

index eab8c09..c24f1e1 100644
@@ -260,6 +260,7 @@ static struct file_system_type ramfs_fs_type = {
 	.name		= "ramfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= ramfs_kill_sb,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 static struct file_system_type rootfs_fs_type = {
 	.name		= "rootfs",

Signature · 1 line

1.7.5.4

There is no backing store to ramfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it.

The memory control group successfully limits how much memory ramfs can consume on any system that cares about a user namespace root using ramfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/ramfs/inode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

fs/ramfs/inode.c +1 -0

index eab8c09..c24f1e1 100644
@@ -260,6 +260,7 @@ static struct file_system_type ramfs_fs_type = {
 	.name		= "ramfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= ramfs_kill_sb,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 static struct file_system_type rootfs_fs_type = {
 	.name		= "rootfs",

Signature · 1 line

1.7.5.4

There is no backing store to tmpfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it. ramfs is safe for the same reasons so allow either flavor of tmpfs to be mounted by a user namespace root user.

The memory control group successfully limits how much memory tmpfs can consume on any system that cares about a user namespace root using tmpfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 mm/shmem.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

mm/shmem.c +2 -0

index 5c90d84..197ca5e 100644
@@ -2766,6 +2766,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= shmem_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)

@@ -2823,6 +2824,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)

Signature · 1 line

1.7.5.4

There is no backing store to tmpfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it. ramfs is safe for the same reasons so allow either flavor of tmpfs to be mounted by a user namespace root user.

The memory control group successfully limits how much memory tmpfs can consume on any system that cares about a user namespace root using tmpfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <[email protected]>

---

 mm/shmem.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

mm/shmem.c +2 -0

index 5c90d84..197ca5e 100644
@@ -2766,6 +2766,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= shmem_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)

@@ -2823,6 +2824,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)

There is no backing store to tmpfs and file creation rules are the same as for any other filesystem so it is semantically safe to allow unprivileged users to mount it. ramfs is safe for the same reasons so allow either flavor of tmpfs to be mounted by a user namespace root user.

The memory control group successfully limits how much memory tmpfs can consume on any system that cares about a user namespace root using tmpfs to exhaust memory the memory control group can be deployed.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

 mm/shmem.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

mm/shmem.c +2 -0

index 5c90d84..197ca5e 100644
@@ -2766,6 +2766,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= shmem_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)

@@ -2823,6 +2824,7 @@ static struct file_system_type shmem_fs_type = {
 	.name		= "tmpfs",
 	.mount		= ramfs_mount,
 	.kill_sb	= kill_litter_super,
+	.fs_flags	= FS_USERNS_MOUNT,
 };
 
 int __init shmem_init(void)