Completing the user namespace

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c +8 -21

-	ns->creator = new->user;
+	ns->owner = owner;
+	ns->group = group;
+	free_uid(new->user);
 	new->user = root_user;
 	new->uid = new->euid = new->suid = new->fsuid = 0;
 	new->gid = new->egid = new->sgid = new->fsgid = 0;

@@ -69,29 +81,15 @@ int create_user_ns(struct cred *new)
 	return 0;
 }
 
-/*
- * Deferred destructor for a user namespace.  This is required because
- * free_user_ns() may be called with uidhash_lock held, but we need to call
- * back to free_uid() which will want to take the lock again.
- */
-static void free_user_ns_work(struct work_struct *work)
+void free_user_ns(struct kref *kref)
 {
 	struct user_namespace *parent, *ns =
-		container_of(work, struct user_namespace, destroyer);
+		container_of(kref, struct user_namespace, kref);
+
 	parent = ns->parent;
-	free_uid(ns->creator);
 	kmem_cache_free(user_ns_cachep, ns);
 	put_user_ns(parent);
 }
-
-void free_user_ns(struct kref *kref)
-{
-	struct user_namespace *ns =
-		container_of(kref, struct user_namespace, kref);
-
-	INIT_WORK(&ns->destroyer, free_user_ns_work);
-	schedule_work(&ns->destroyer);
-}
 EXPORT_SYMBOL(free_user_ns);
 
 uid_t user_ns_map_uid(struct user_namespace *to, const struct cred *cred, uid_t uid)

@@ -101,12 +99,11 @@ uid_t user_ns_map_uid(struct user_namespace *to, const struct cred *cred, uid_t
 	if (likely(to == cred->user_ns))
 		return uid;
 
-
 	/* Is cred->user the creator of the target user_ns
 	 * or the creator of one of it's parents?
 	 */
 	for ( tmp = to; tmp != &init_user_ns; tmp = tmp->parent ) {
-		if (cred->user == tmp->creator) {
+		if (uid_eq(cred->user->uid, tmp->owner)) {
 			return (uid_t)0;
 		}
 	}

@@ -126,7 +123,7 @@ gid_t user_ns_map_gid(struct user_namespace *to, const struct cred *cred, gid_t
 	 * or the creator of one of it's parents?
 	 */
 	for ( tmp = to; tmp != &init_user_ns; tmp = tmp->parent ) {
-		if (cred->user == tmp->creator) {
+		if (uid_eq(cred->user->uid, tmp->owner)) {
 			return (gid_t)0;
 		}
 	}

security/commoncap.c +3 -2

index 435d074..f2399d8 100644
@@ -76,8 +76,9 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
 		int cap, int audit)
 {
 	for (;;) {
-		/* The creator of the user namespace has all caps. */
-		if (targ_ns != &init_user_ns && targ_ns->creator == cred->user)
+		/* The owner of the user namespace has all caps. */
+		if (targ_ns != &init_user_ns && uid_eq(targ_ns->owner,
+						       make_kuid(cred->user_ns, cred->euid)))
 			return 0;
 
 		/* Do we have the necessary capabilities? */

Signature · 6 lines

1.7.2.5

_______________________________________________
Containers mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/containers

Quoted 90 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 2 lines

kernel/user_namespace.c +0 -0

 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 91 lines · Serge E. Hallyn

Quoted 90 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 

Quoted 5 lines

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 2 lines

kernel/user_namespace.c +0 -0

 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Quoted 99 lines · Eric W. Biederman

Quoted 96 lines · Serge E. Hallyn

Quoted 89 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 3 lines

kernel/user_namespace.c +0 -0

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 
 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 3 lines

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Perhaps that suffices, but I'm not convinced. The struct cred is pinning it's own ns, but if t1 does clone(CLONE_NEWUSER) to produce t2, which does the same to procduce t3, and then t2 exits, I'm not seeing what will pin t2's userns.

Quoted 100 lines · Serge E. Hallyn

Quoted 99 lines · Eric W. Biederman

Quoted 96 lines · Serge E. Hallyn

Quoted 89 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 3 lines

kernel/user_namespace.c +0 -0

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 
 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 3 lines

Quoted 3 lines

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Make that patch 8 not patch 7:
"userns: Add an explicit reference to the parent user namespace" Perhaps the patch number reference pointed you to look at the wrong code.

Quoted 4 lines

Perhaps that suffices, but I'm not convinced. The struct cred is pinning it's own ns, but if t1 does clone(CLONE_NEWUSER) to produce t2, which does the same to procduce t3, and then t2 exits, I'm not seeing what will pin t2's userns.

t3's userns hold's a reference to the departed t2's userns. t2's userns hold's a reference to t1's userns.

free_user_ns does put that userns reference.

It is all there and explict. Usernamespaces refer directly to each

other. That was all needed to get struct user out of the usernamespace game.

Eric

Quoted 102 lines · Eric W. Biederman

Quoted 100 lines · Serge E. Hallyn

Quoted 99 lines · Eric W. Biederman

Quoted 96 lines · Serge E. Hallyn

Quoted 89 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 3 lines

kernel/user_namespace.c +0 -0

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 
 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Make that patch 8 not patch 7:
"userns: Add an explicit reference to the parent user namespace" Perhaps the patch number reference pointed you to look at the wrong code.

D'oh, yup. That explains it better.

And so parent_userns keeps the refcount from the cred 'new' after new->ns = ns; That works, thanks.

Quoted 11 lines

Quoted 4 lines

Perhaps that suffices, but I'm not convinced. The struct cred is pinning it's own ns, but if t1 does clone(CLONE_NEWUSER) to produce t2, which does the same to procduce t3, and then t2 exits, I'm not seeing what will pin t2's userns.

t3's userns hold's a reference to the departed t2's userns. t2's userns hold's a reference to t1's userns.

free_user_ns does put that userns reference.

It is all there and explict. Usernamespaces refer directly to each

Actually can we make it just one tinge more explicit, and put a comment above the 'new->user_ns = ns'? There's currently the comment

   /* Leave the reference to our user_ns with the new cred */

But that's about the initial refcount on the new ns. Perhaps change that to:

/*
* Leave the reference to our new user_ns with the new cred,
* and leave the reference on the old ns to pin new->parent_ns */

Quoted 4 lines

other. That was all needed to get struct user out of the usernamespace game.

Quoted 1 line

Eric

Thanks, Eric. So then

Acked-by Serge Hallyn <[email protected]>

which, given the other nits are addressed, should cover the whole set with my acks.

Quoted 135 lines · Serge E. Hallyn

Quoted 102 lines · Eric W. Biederman

Quoted 100 lines · Serge E. Hallyn

Quoted 99 lines · Eric W. Biederman

Quoted 96 lines · Serge E. Hallyn

Quoted 89 lines · Eric W. Beiderman

From: Eric W. Biederman <[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 3 lines

kernel/user_namespace.c +0 -0

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 
 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Make that patch 8 not patch 7:
"userns: Add an explicit reference to the parent user namespace" Perhaps the patch number reference pointed you to look at the wrong code.

D'oh, yup. That explains it better.

And so parent_userns keeps the refcount from the cred 'new' after new->ns = ns; That works, thanks.

Quoted 11 lines

Quoted 4 lines

Perhaps that suffices, but I'm not convinced. The struct cred is pinning it's own ns, but if t1 does clone(CLONE_NEWUSER) to produce t2, which does the same to procduce t3, and then t2 exits, I'm not seeing what will pin t2's userns.

t3's userns hold's a reference to the departed t2's userns. t2's userns hold's a reference to t1's userns.

free_user_ns does put that userns reference.

It is all there and explict. Usernamespaces refer directly to each

Actually can we make it just one tinge more explicit, and put a comment above the 'new->user_ns = ns'? There's currently the comment

   /* Leave the reference to our user_ns with the new cred */

But that's about the initial refcount on the new ns. Perhaps change that to:

Quoted 4 lines

/*
* Leave the reference to our new user_ns with the new cred,
* and leave the reference on the old ns to pin new->parent_ns */

I have added the following comment. Hopefully that makes it clearer. */

	/* Leave the new->user_ns reference with the new user namespace. */

Quoted 7 lines

Quoted 4 lines

other. That was all needed to get struct user out of the usernamespace game.

Eric

Thanks, Eric. So then

Quoted 4 lines

Acked-by Serge Hallyn <[email protected]>

which, given the other nits are addressed, should cover the whole set with my acks.

Hmm. I don't have a record of you looking at my patch 23.
"userns: Convert setting and getting uid and gid system calls to use kuid and kgid"

Quoted 102 lines · Eric W. Biederman

Quoted 100 lines · Serge E. Hallyn

Quoted 99 lines · Eric W. Biederman

Quoted 96 lines · Serge E. Hallyn

Quoted 89 lines · Eric W. Beiderman

From: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 3 lines

kernel/user_namespace.c +0 -0

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 
 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Yes we do have to explicitly reference count the parent namespace. But that happened in the patch 7:
"userns: Add an explicit reference to the parent user namespace"

Make that patch 8 not patch 7:
"userns: Add an explicit reference to the parent user namespace" Perhaps the patch number reference pointed you to look at the wrong code.

D'oh, yup. That explains it better.

And so parent_userns keeps the refcount from the cred 'new' after new->ns = ns; That works, thanks.

Quoted 11 lines

Quoted 4 lines

Perhaps that suffices, but I'm not convinced. The struct cred is pinning it's own ns, but if t1 does clone(CLONE_NEWUSER) to produce t2, which does the same to procduce t3, and then t2 exits, I'm not seeing what will pin t2's userns.

t3's userns hold's a reference to the departed t2's userns. t2's userns hold's a reference to t1's userns.

free_user_ns does put that userns reference.

It is all there and explict. Usernamespaces refer directly to each

Actually can we make it just one tinge more explicit, and put a comment above the 'new->user_ns = ns'? There's currently the comment

   /* Leave the reference to our user_ns with the new cred */

But that's about the initial refcount on the new ns. Perhaps change that to:

/*
* Leave the reference to our new user_ns with the new cred,
* and leave the reference on the old ns to pin new->parent_ns */

Quoted 4 lines

other. That was all needed to get struct user out of the usernamespace game.

Quoted 1 line

Eric

Thanks, Eric. So then

Acked-by Serge Hallyn <[email protected]>

which, given the other nits are addressed, should cover the whole set with my acks.

Quoted 90 lines · Eric W. Beiderman

From: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

@@ -43,7 +53,9 @@ int create_user_ns(struct cred *new)
 

kernel/user_namespace.c int create_user_ns(struct cred *new) · line 55

Quoted 2 lines

kernel/user_namespace.c +0 -0

 	/* set the new root user in the credentials under preparation */
 	ns->parent = parent_ns;

I think in the past the creator cred pinned the ns->parent. Do you now need to explicitly pin ns->parent (and release it in free_user_ns())?

From: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

- Transform userns->creator from a user_struct reference to a simple kuid_t, kgid_t pair.

In cap_capable this allows the check to see if we are the creator of a namespace to become the classic suser style euid permission check.

This allows us to remove the need for a struct cred in the mapping functions and still be able to dispaly the user namespace creators uid and gid as 0.

- Remove the now unnecessary delayed_work in free_user_ns.

All that is left for free_user_ns to do is to call kmem_cache_free and put_user_ns. Those functions can be called in any context so call them directly from free_user_ns removing the need for delayed work.

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

---
 include/linux/user_namespace.h |    4 ++--
 kernel/user.c                  |    7 ++++---
 kernel/user_namespace.c        |   39 ++++++++++++++++++---------------------
 security/commoncap.c           |    5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

include/linux/user_namespace.h +2 -2

index d767508..8a391bd 100644
@@ -9,8 +9,8 @@
 struct user_namespace {
 	struct kref		kref;
 	struct user_namespace	*parent;
-	struct user_struct	*creator;
-	struct work_struct	destroyer;
+	kuid_t			owner;
+	kgid_t			group;
 };
 
 extern struct user_namespace init_user_ns;

kernel/user.c +4 -3

index 025077e..cff3856 100644
@@ -25,7 +25,8 @@ struct user_namespace init_user_ns = {
 	.kref = {
 		.refcount	= ATOMIC_INIT(3),
 	},
-	.creator = &root_user,
+	.owner = GLOBAL_ROOT_UID,
+	.group = GLOBAL_ROOT_GID,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

@@ -54,9 +55,9 @@ struct hlist_head uidhash_table[UIDHASH_SZ];
  */
 static DEFINE_SPINLOCK(uidhash_lock);
 
-/* root_user.__count is 2, 1 for init task cred, 1 for init_user_ns->user_ns */
+/* root_user.__count is 1, for init task cred */
 struct user_struct root_user = {
-	.__count	= ATOMIC_INIT(2),
+	.__count	= ATOMIC_INIT(1),
 	.processes	= ATOMIC_INIT(1),
 	.files		= ATOMIC_INIT(0),
 	.sigpending	= ATOMIC_INIT(0),

kernel/user_namespace.c +10 -0

index 898e973..f69741a 100644
@@ -27,6 +27,16 @@ int create_user_ns(struct cred *new)
 {
 	struct user_namespace *ns, *parent_ns = new->user_ns;
 	struct user_struct *root_user;
+	kuid_t owner = make_kuid(new->user_ns, new->euid);
+	kgid_t group = make_kgid(new->user_ns, new->egid);
+
+	/* The creator needs a mapping in the parent user namespace
+	 * or else we won't be able to reasonably tell userspace who
+	 * created a user_namespace.
+	 */
+	if (!kuid_has_mapping(parent_ns, owner) ||
+	    !kgid_has_mapping(parent_ns, group))
+		return -EPERM;
 
 	ns = kmem_cache_alloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)

kernel/user_namespace.c +8 -21

-	ns->creator = new->user;
+	ns->owner = owner;
+	ns->group = group;
+	free_uid(new->user);
 	new->user = root_user;
 	new->uid = new->euid = new->suid = new->fsuid = 0;
 	new->gid = new->egid = new->sgid = new->fsgid = 0;

@@ -69,29 +81,15 @@ int create_user_ns(struct cred *new)
 	return 0;
 }
 
-/*
- * Deferred destructor for a user namespace.  This is required because
- * free_user_ns() may be called with uidhash_lock held, but we need to call
- * back to free_uid() which will want to take the lock again.
- */
-static void free_user_ns_work(struct work_struct *work)
+void free_user_ns(struct kref *kref)
 {
 	struct user_namespace *parent, *ns =
-		container_of(work, struct user_namespace, destroyer);
+		container_of(kref, struct user_namespace, kref);
+
 	parent = ns->parent;
-	free_uid(ns->creator);
 	kmem_cache_free(user_ns_cachep, ns);
 	put_user_ns(parent);
 }
-
-void free_user_ns(struct kref *kref)
-{
-	struct user_namespace *ns =
-		container_of(kref, struct user_namespace, kref);
-
-	INIT_WORK(&ns->destroyer, free_user_ns_work);
-	schedule_work(&ns->destroyer);
-}
 EXPORT_SYMBOL(free_user_ns);
 
 uid_t user_ns_map_uid(struct user_namespace *to, const struct cred *cred, uid_t uid)

@@ -101,12 +99,11 @@ uid_t user_ns_map_uid(struct user_namespace *to, const struct cred *cred, uid_t
 	if (likely(to == cred->user_ns))
 		return uid;
 
-
 	/* Is cred->user the creator of the target user_ns
 	 * or the creator of one of it's parents?
 	 */
 	for ( tmp = to; tmp != &init_user_ns; tmp = tmp->parent ) {
-		if (cred->user == tmp->creator) {
+		if (uid_eq(cred->user->uid, tmp->owner)) {
 			return (uid_t)0;
 		}
 	}

@@ -126,7 +123,7 @@ gid_t user_ns_map_gid(struct user_namespace *to, const struct cred *cred, gid_t
 	 * or the creator of one of it's parents?
 	 */
 	for ( tmp = to; tmp != &init_user_ns; tmp = tmp->parent ) {
-		if (cred->user == tmp->creator) {
+		if (uid_eq(cred->user->uid, tmp->owner)) {
 			return (gid_t)0;
 		}
 	}

security/commoncap.c +3 -2

index 435d074..f2399d8 100644
@@ -76,8 +76,9 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
 		int cap, int audit)
 {
 	for (;;) {
-		/* The creator of the user namespace has all caps. */
-		if (targ_ns != &init_user_ns && targ_ns->creator == cred->user)
+		/* The owner of the user namespace has all caps. */
+		if (targ_ns != &init_user_ns && uid_eq(targ_ns->owner,
+						       make_kuid(cred->user_ns, cred->euid)))
 			return 0;
 
 		/* Do we have the necessary capabilities? */

Signature · 6 lines

1.7.2.5

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

From: Eric W. Biederman <[email protected]>

As a first step to converting struct cred to be all kuid_t and kgid_t values convert the group values stored in group_info to always be
kgid_t values. Unless user namespaces are used this change should
have no effect.

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

---
 arch/s390/kernel/compat_linux.c   |   13 ++++++++-
 fs/nfsd/auth.c                    |    5 ++-
 fs/proc/array.c                   |    5 +++-
 include/linux/cred.h              |    9 ++++---
 kernel/groups.c                   |   48 +++++++++++++++++++-----------------
 kernel/uid16.c                    |   14 +++++++++-
 net/ipv4/ping.c                   |   11 ++++++--
 net/sunrpc/auth_generic.c         |    4 +-
 net/sunrpc/auth_gss/svcauth_gss.c |    7 ++++-
 net/sunrpc/auth_unix.c            |   15 ++++++++---
 net/sunrpc/svcauth_unix.c         |   18 ++++++++++---
 security/keys/permission.c        |    3 +-
 12 files changed, 103 insertions(+), 49 deletions(-)

arch/s390/kernel/compat_linux.c +4 -1

index ab64bdb..5baac18 100644
@@ -173,11 +173,14 @@ asmlinkage long sys32_setfsgid16(u16 gid)
 
 static int groups16_to_user(u16 __user *grouplist, struct group_info *group_info)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	int i;
 	u16 group;
+	kgid_t kgid;
 
 	for (i = 0; i < group_info->ngroups; i++) {
-		group = (u16)GROUP_AT(group_info, i);
+		kgid = GROUP_AT(group_info, i);
+		group = (u16)from_kgid_munged(user_ns, kgid);
 		if (put_user(group, grouplist+i))
 			return -EFAULT;
 	}

arch/s390/kernel/compat_linux.c +6 -1

 	for (i = 0; i < group_info->ngroups; i++) {
 		if (get_user(group, grouplist+i))
 			return  -EFAULT;
-		GROUP_AT(group_info, i) = (gid_t)group;
+
+		kgid = make_kgid(user_ns, (gid_t)group);
+		if (!gid_valid(kgid))
+			return -EINVAL;
+
+		GROUP_AT(group_info, i) = kgid;
 	}
 
 	return 0;

fs/nfsd/auth.c +3 -2

index 79717a4..204438c 100644
@@ -1,6 +1,7 @@
 /* Copyright (C) 1995, 1996 Olaf Kirch <[email protected]> */
 
 #include <linux/sched.h>
+#include <linux/user_namespace.h>
 #include "nfsd.h"
 #include "auth.h"
 

@@ -56,8 +57,8 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
 			goto oom;
 
 		for (i = 0; i < rqgi->ngroups; i++) {
-			if (!GROUP_AT(rqgi, i))
-				GROUP_AT(gi, i) = exp->ex_anon_gid;
+			if (gid_eq(GLOBAL_ROOT_GID, GROUP_AT(rqgi, i)))
+				GROUP_AT(gi, i) = make_kgid(&init_user_ns, exp->ex_anon_gid);
 			else
 				GROUP_AT(gi, i) = GROUP_AT(rqgi, i);
 		}

fs/proc/array.c +4 -1

index f9bd395..36a0a91 100644
@@ -81,6 +81,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/ptrace.h>
 #include <linux/tracehook.h>
+#include <linux/user_namespace.h>
 
 #include <asm/pgtable.h>
 #include <asm/processor.h>

@@ -161,6 +162,7 @@ static inline const char *get_task_state(struct task_struct *tsk)
 static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
 				struct pid *pid, struct task_struct *p)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	struct group_info *group_info;
 	int g;
 	struct fdtable *fdt = NULL;

@@ -205,7 +207,8 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
 	task_unlock(p);
 
 	for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
-		seq_printf(m, "%d ", GROUP_AT(group_info, g));
+		seq_printf(m, "%d ",
+			   from_kgid_munged(user_ns, GROUP_AT(group_info, g)));
 	put_cred(cred);
 
 	seq_putc(m, '\n');

include/linux/cred.h +5 -4

index 2c60ec8..0ab3cda 100644
@@ -17,6 +17,7 @@
 #include <linux/key.h>
 #include <linux/selinux.h>
 #include <linux/atomic.h>
+#include <linux/uidgid.h>
 
 struct user_struct;
 struct cred;

@@ -26,14 +27,14 @@ struct inode;
  * COW Supplementary groups list
  */
 #define NGROUPS_SMALL		32
-#define NGROUPS_PER_BLOCK	((unsigned int)(PAGE_SIZE / sizeof(gid_t)))
+#define NGROUPS_PER_BLOCK	((unsigned int)(PAGE_SIZE / sizeof(kgid_t)))
 
 struct group_info {
 	atomic_t	usage;
 	int		ngroups;
 	int		nblocks;
-	gid_t		small_block[NGROUPS_SMALL];
-	gid_t		*blocks[0];
+	kgid_t		small_block[NGROUPS_SMALL];
+	kgid_t		*blocks[0];
 };
 
 /**

@@ -66,7 +67,7 @@ extern struct group_info init_groups;
 extern void groups_free(struct group_info *);
 extern int set_current_groups(struct group_info *);
 extern int set_groups(struct cred *, struct group_info *);
-extern int groups_search(const struct group_info *, gid_t);
+extern int groups_search(const struct group_info *, kgid_t);
 
 /* access the groups "array" with this macro */
 #define GROUP_AT(gi, i) \

kernel/groups.c +25 -23

index 99b53d1..84156f2 100644
@@ -31,7 +31,7 @@ struct group_info *groups_alloc(int gidsetsize)
 		group_info->blocks[0] = group_info->small_block;
 	else {
 		for (i = 0; i < nblocks; i++) {
-			gid_t *b;
+			kgid_t *b;
 			b = (void *)__get_free_page(GFP_USER);
 			if (!b)
 				goto out_undo_partial_alloc;

@@ -66,18 +66,15 @@ EXPORT_SYMBOL(groups_free);
 static int groups_to_user(gid_t __user *grouplist,
 			  const struct group_info *group_info)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	int i;
 	unsigned int count = group_info->ngroups;
 
-	for (i = 0; i < group_info->nblocks; i++) {
-		unsigned int cp_count = min(NGROUPS_PER_BLOCK, count);
-		unsigned int len = cp_count * sizeof(*grouplist);
-
-		if (copy_to_user(grouplist, group_info->blocks[i], len))
+	for (i = 0; i < count; i++) {
+		gid_t gid;
+		gid = from_kgid_munged(user_ns, GROUP_AT(group_info, i));
+		if (put_user(gid, grouplist+i))
 			return -EFAULT;
-
-		grouplist += NGROUPS_PER_BLOCK;
-		count -= cp_count;
 	}
 	return 0;
 }

@@ -86,18 +83,21 @@ static int groups_to_user(gid_t __user *grouplist,
 static int groups_from_user(struct group_info *group_info,
     gid_t __user *grouplist)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	int i;
 	unsigned int count = group_info->ngroups;
 
-	for (i = 0; i < group_info->nblocks; i++) {
-		unsigned int cp_count = min(NGROUPS_PER_BLOCK, count);
-		unsigned int len = cp_count * sizeof(*grouplist);
-
-		if (copy_from_user(group_info->blocks[i], grouplist, len))
+	for (i = 0; i < count; i++) {
+		gid_t gid;
+		kgid_t kgid;
+		if (get_user(gid, grouplist+i))
 			return -EFAULT;
 
-		grouplist += NGROUPS_PER_BLOCK;
-		count -= cp_count;
+		kgid = make_kgid(user_ns, gid);
+		if (!gid_valid(kgid))
+			return -EINVAL;
+
+		GROUP_AT(group_info, i) = kgid;
 	}
 	return 0;
 }

@@ -117,9 +117,9 @@ static void groups_sort(struct group_info *group_info)
 		for (base = 0; base < max; base++) {
 			int left = base;
 			int right = left + stride;
-			gid_t tmp = GROUP_AT(group_info, right);
+			kgid_t tmp = GROUP_AT(group_info, right);
 
-			while (left >= 0 && GROUP_AT(group_info, left) > tmp) {
+			while (left >= 0 && gid_gt(GROUP_AT(group_info, left), tmp)) {
 				GROUP_AT(group_info, right) =
 				    GROUP_AT(group_info, left);
 				right = left;

@@ -132,7 +132,7 @@ static void groups_sort(struct group_info *group_info)
 }
 
 /* a simple bsearch */
-int groups_search(const struct group_info *group_info, gid_t grp)
+int groups_search(const struct group_info *group_info, kgid_t grp)
 {
 	unsigned int left, right;
 

@@ -143,9 +143,9 @@ int groups_search(const struct group_info *group_info, gid_t grp)
 	right = group_info->ngroups;
 	while (left < right) {
 		unsigned int mid = (left+right)/2;
-		if (grp > GROUP_AT(group_info, mid))
+		if (gid_gt(grp, GROUP_AT(group_info, mid)))
 			left = mid + 1;
-		else if (grp < GROUP_AT(group_info, mid))
+		else if (gid_lt(grp, GROUP_AT(group_info, mid)))
 			right = mid;
 		else
 			return 1;

@@ -262,7 +262,8 @@ int in_group_p(gid_t grp)
 	int retval = 1;
 
 	if (grp != cred->fsgid)
-		retval = groups_search(cred->group_info, grp);
+		retval = groups_search(cred->group_info,
+				       make_kgid(cred->user_ns, grp));
 	return retval;
 }
 

@@ -274,7 +275,8 @@ int in_egroup_p(gid_t grp)
 	int retval = 1;
 
 	if (grp != cred->egid)
-		retval = groups_search(cred->group_info, grp);
+		retval = groups_search(cred->group_info,
+				       make_kgid(cred->user_ns, grp));
 	return retval;
 }
 

kernel/uid16.c +12 -2

index 51c6e89..e530bc3 100644
@@ -134,11 +134,14 @@ SYSCALL_DEFINE1(setfsgid16, old_gid_t, gid)
 static int groups16_to_user(old_gid_t __user *grouplist,
     struct group_info *group_info)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	int i;
 	old_gid_t group;
+	kgid_t kgid;
 
 	for (i = 0; i < group_info->ngroups; i++) {
-		group = high2lowgid(GROUP_AT(group_info, i));
+		kgid = GROUP_AT(group_info, i);
+		group = high2lowgid(from_kgid_munged(user_ns, kgid));
 		if (put_user(group, grouplist+i))
 			return -EFAULT;
 	}

@@ -149,13 +152,20 @@ static int groups16_to_user(old_gid_t __user *grouplist,
 static int groups16_from_user(struct group_info *group_info,
     old_gid_t __user *grouplist)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	int i;
 	old_gid_t group;
+	kgid_t kgid;
 
 	for (i = 0; i < group_info->ngroups; i++) {
 		if (get_user(group, grouplist+i))
 			return  -EFAULT;
-		GROUP_AT(group_info, i) = low2highgid(group);
+
+		kgid = make_kgid(user_ns, low2highgid(group));
+		if (!gid_valid(kgid))
+			return -EINVAL;
+
+		GROUP_AT(group_info, i) = kgid;
 	}
 
 	return 0;

net/ipv4/ping.c +8 -3

index 50009c7..9d3044f 100644
@@ -205,17 +205,22 @@ static int ping_init_sock(struct sock *sk)
 	gid_t range[2];
 	struct group_info *group_info = get_current_groups();
 	int i, j, count = group_info->ngroups;
+	kgid_t low, high;
 
 	inet_get_ping_group_range_net(net, range, range+1);
+	low = make_kgid(&init_user_ns, range[0]);
+	high = make_kgid(&init_user_ns, range[1]);
+	if (!gid_valid(low) || !gid_valid(high) || gid_lt(high, low))
+		return -EACCES;
+
 	if (range[0] <= group && group <= range[1])
 		return 0;
 
 	for (i = 0; i < group_info->nblocks; i++) {
 		int cp_count = min_t(int, NGROUPS_PER_BLOCK, count);
-
 		for (j = 0; j < cp_count; j++) {
-			group = group_info->blocks[i][j];
-			if (range[0] <= group && group <= range[1])
+			kgid_t gid = group_info->blocks[i][j];
+			if (gid_lte(low, gid) && gid_lte(gid, high))
 				return 0;
 		}
 

net/sunrpc/auth_generic.c +2 -2

index 75762f3..6ed6f20 100644
@@ -160,8 +160,8 @@ generic_match(struct auth_cred *acred, struct rpc_cred *cred, int flags)
 	if (gcred->acred.group_info->ngroups != acred->group_info->ngroups)
 		goto out_nomatch;
 	for (i = 0; i < gcred->acred.group_info->ngroups; i++) {
-		if (GROUP_AT(gcred->acred.group_info, i) !=
-				GROUP_AT(acred->group_info, i))
+		if (!gid_eq(GROUP_AT(gcred->acred.group_info, i),
+				GROUP_AT(acred->group_info, i)))
 			goto out_nomatch;
 	}
 out_match:

net/sunrpc/auth_gss/svcauth_gss.c +6 -1

index 1600cfb..28b62db 100644
@@ -41,6 +41,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/user_namespace.h>
 
 #include <linux/sunrpc/auth_gss.h>
 #include <linux/sunrpc/gss_err.h>

@@ -470,9 +471,13 @@ static int rsc_parse(struct cache_detail *cd,
 		status = -EINVAL;
 		for (i=0; i<N; i++) {
 			gid_t gid;
+			kgid_t kgid;
 			if (get_int(&mesg, &gid))
 				goto out;
-			GROUP_AT(rsci.cred.cr_group_info, i) = gid;
+			kgid = make_kgid(&init_user_ns, gid);
+			if (!gid_valid(kgid))
+				goto out;
+			GROUP_AT(rsci.cred.cr_group_info, i) = kgid;
 		}
 
 		/* mech name */

net/sunrpc/auth_unix.c +11 -4

index e50502d..52c5abd 100644
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/auth.h>
+#include <linux/user_namespace.h>
 
 #define NFS_NGROUPS	16
 

@@ -78,8 +79,11 @@ unx_create_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
 		groups = NFS_NGROUPS;
 
 	cred->uc_gid = acred->gid;
-	for (i = 0; i < groups; i++)
-		cred->uc_gids[i] = GROUP_AT(acred->group_info, i);
+	for (i = 0; i < groups; i++) {
+		gid_t gid;
+		gid = from_kgid(&init_user_ns, GROUP_AT(acred->group_info, i));
+		cred->uc_gids[i] = gid;
+	}
 	if (i < NFS_NGROUPS)
 		cred->uc_gids[i] = NOGROUP;
 

@@ -126,9 +130,12 @@ unx_match(struct auth_cred *acred, struct rpc_cred *rcred, int flags)
 		groups = acred->group_info->ngroups;
 	if (groups > NFS_NGROUPS)
 		groups = NFS_NGROUPS;
-	for (i = 0; i < groups ; i++)
-		if (cred->uc_gids[i] != GROUP_AT(acred->group_info, i))
+	for (i = 0; i < groups ; i++) {
+		gid_t gid;
+		gid = from_kgid(&init_user_ns, GROUP_AT(acred->group_info, i));
+		if (cred->uc_gids[i] != gid)
 			return 0;
+	}
 	if (groups < NFS_NGROUPS &&
 	    cred->uc_gids[groups] != NOGROUP)
 		return 0;

net/sunrpc/svcauth_unix.c +14 -4

index 521d8f7..71ec853 100644
@@ -14,6 +14,7 @@
 #include <net/sock.h>
 #include <net/ipv6.h>
 #include <linux/kernel.h>
+#include <linux/user_namespace.h>
 #define RPCDBG_FACILITY	RPCDBG_AUTH
 
 #include <linux/sunrpc/clnt.h>

@@ -530,11 +531,15 @@ static int unix_gid_parse(struct cache_detail *cd,
 
 	for (i = 0 ; i < gids ; i++) {
 		int gid;
+		kgid_t kgid;
 		rv = get_int(&mesg, &gid);
 		err = -EINVAL;
 		if (rv)
 			goto out;
-		GROUP_AT(ug.gi, i) = gid;
+		kgid = make_kgid(&init_user_ns, gid);
+		if (!gid_valid(kgid))
+			goto out;
+		GROUP_AT(ug.gi, i) = kgid;
 	}
 
 	ugp = unix_gid_lookup(cd, uid);

@@ -563,6 +568,7 @@ static int unix_gid_show(struct seq_file *m,
 			 struct cache_detail *cd,
 			 struct cache_head *h)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	struct unix_gid *ug;
 	int i;
 	int glen;

@@ -580,7 +586,7 @@ static int unix_gid_show(struct seq_file *m,
 
 	seq_printf(m, "%u %d:", ug->uid, glen);
 	for (i = 0; i < glen; i++)
-		seq_printf(m, " %d", GROUP_AT(ug->gi, i));
+		seq_printf(m, " %d", from_kgid_munged(user_ns, GROUP_AT(ug->gi, i)));
 	seq_printf(m, "\n");
 	return 0;
 }

@@ -831,8 +837,12 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
 	cred->cr_group_info = groups_alloc(slen);
 	if (cred->cr_group_info == NULL)
 		return SVC_CLOSE;
-	for (i = 0; i < slen; i++)
-		GROUP_AT(cred->cr_group_info, i) = svc_getnl(argv);
+	for (i = 0; i < slen; i++) {
+		kgid_t kgid = make_kgid(&init_user_ns, svc_getnl(argv));
+		if (!gid_valid(kgid))
+			goto badcred;
+		GROUP_AT(cred->cr_group_info, i) = kgid;
+	}
 	if (svc_getu32(argv) != htonl(RPC_AUTH_NULL) || svc_getu32(argv) != 0) {
 		*authp = rpc_autherr_badverf;
 		return SVC_DENIED;

security/keys/permission.c +2 -1

index e146cbd..5442900 100644
@@ -53,7 +53,8 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 			goto use_these_perms;
 		}
 
-		ret = groups_search(cred->group_info, key->gid);
+		ret = groups_search(cred->group_info,
+				    make_kgid(current_user_ns(), key->gid));
 		if (ret) {
 			kperm = key->perm >> 8;
 			goto use_these_perms;

Signature · 6 lines

1.7.2.5

_______________________________________________
Containers mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/containers