[PATCH 1/4] keys: distinguish per-uid keys in different namespaces

[PATCH 1/4] keys: distinguish per-uid keys in different namespaces

[Serge E. Hallyn]

per-uid keys were looked by uid only.  Use the user namespace
to distinguish the same uid in different namespaces.

This does not address key_permission.  So a task can for instance
try to join a keyring owned by the same uid in another namespace.
That will be handled by a separate patch.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 kernel/user.c                |    2 +-
 security/keys/internal.h     |    4 +++-
 security/keys/key.c          |   11 +++++++++--
 security/keys/keyctl.c       |    2 +-
 security/keys/process_keys.c |    2 ++
 security/keys/request_key.c  |    2 +-
 6 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/kernel/user.c b/kernel/user.c
index 6608a3d..fe326a6 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -20,7 +20,7 @@
 
 struct user_namespace init_user_ns = {
 	.kref = {
-		.refcount	= ATOMIC_INIT(1),
+		.refcount	= ATOMIC_INIT(2),
 	},
 	.creator = &root_user,
 };
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 81932ab..9fb679c 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -53,6 +53,7 @@ struct key_user {
 	atomic_t		nkeys;		/* number of keys */
 	atomic_t		nikeys;		/* number of instantiated keys */
 	uid_t			uid;
+	struct user_namespace	*user_ns;
 	int			qnkeys;		/* number of keys allocated to this user */
 	int			qnbytes;	/* number of bytes allocated to this user */
 };
@@ -61,7 +62,8 @@ extern struct rb_root	key_user_tree;
 extern spinlock_t	key_user_lock;
 extern struct key_user	root_key_user;
 
-extern struct key_user *key_user_lookup(uid_t uid);
+extern struct key_user *key_user_lookup(uid_t uid,
+					struct user_namespace *user_ns);
 extern void key_user_put(struct key_user *user);
 
 /*
diff --git a/security/keys/key.c b/security/keys/key.c
index f76c8a5..4a1297d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -18,6 +18,7 @@
 #include <linux/workqueue.h>
 #include <linux/random.h>
 #include <linux/err.h>
+#include <linux/user_namespace.h>
 #include "internal.h"
 
 static struct kmem_cache	*key_jar;
@@ -60,7 +61,7 @@ void __key_check(const struct key *key)
  * get the key quota record for a user, allocating a new record if one doesn't
  * already exist
  */
-struct key_user *key_user_lookup(uid_t uid)
+struct key_user *key_user_lookup(uid_t uid, struct user_namespace *user_ns)
 {
 	struct key_user *candidate = NULL, *user;
 	struct rb_node *parent = NULL;
@@ -79,6 +80,10 @@ struct key_user *key_user_lookup(uid_t uid)
 			p = &(*p)->rb_left;
 		else if (uid > user->uid)
 			p = &(*p)->rb_right;
+		else if (user_ns < user->user_ns)
+			p = &(*p)->rb_left;
+		else if (user_ns > user->user_ns)
+			p = &(*p)->rb_right;
 		else
 			goto found;
 	}
@@ -106,6 +111,7 @@ struct key_user *key_user_lookup(uid_t uid)
 	atomic_set(&candidate->nkeys, 0);
 	atomic_set(&candidate->nikeys, 0);
 	candidate->uid = uid;
+	candidate->user_ns = get_user_ns(user_ns);
 	candidate->qnkeys = 0;
 	candidate->qnbytes = 0;
 	spin_lock_init(&candidate->lock);
@@ -136,6 +142,7 @@ void key_user_put(struct key_user *user)
 	if (atomic_dec_and_lock(&user->usage, &key_user_lock)) {
 		rb_erase(&user->node, &key_user_tree);
 		spin_unlock(&key_user_lock);
+		put_user_ns(user->user_ns);
 
 		kfree(user);
 	}
@@ -234,7 +241,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	quotalen = desclen + type->def_datalen;
 
 	/* get hold of the key tracking for this user */
-	user = key_user_lookup(uid);
+	user = key_user_lookup(uid, cred->user->user_ns);
 	if (!user)
 		goto no_memory_1;
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 7c72baa..db4c029 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -725,7 +725,7 @@ long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
 	/* change the UID */
 	if (uid != (uid_t) -1 && uid != key->uid) {
 		ret = -ENOMEM;
-		newowner = key_user_lookup(uid);
+		newowner = key_user_lookup(uid, current_user_ns());
 		if (!newowner)
 			goto error_put;
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 2f5d89e..276d278 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -17,6 +17,7 @@
 #include <linux/fs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/user_namespace.h>
 #include <asm/uaccess.h>
 #include "internal.h"
 
@@ -34,6 +35,7 @@ struct key_user root_key_user = {
 	.nkeys		= ATOMIC_INIT(2),
 	.nikeys		= ATOMIC_INIT(2),
 	.uid		= 0,
+	.user_ns	= &init_user_ns,
 };
 
 /*****************************************************************************/
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 0e04f72..22a3158 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -365,7 +365,7 @@ static struct key *construct_key_and_link(struct key_type *type,
 
 	kenter("");
 
-	user = key_user_lookup(current_fsuid());
+	user = key_user_lookup(current_fsuid(), current_user_ns());
 	if (!user)
 		return ERR_PTR(-ENOMEM);
 
-- 
1.5.4.3

[PATCH 2/4] keys: consider user namespace in key_permission

[Serge E. Hallyn]

If a key is owned by another user namespace, then treat the
key as though it is owned by both another uid and gid.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/keys/permission.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/security/keys/permission.c b/security/keys/permission.c
index 5d9fc7b..0ed802c 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -35,6 +35,9 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 
 	key = key_ref_to_ptr(key_ref);
 
+	if (key->user->user_ns != cred->user->user_ns)
+		goto use_other_perms;
+
 	/* use the second 8-bits of permissions for keys the caller owns */
 	if (key->uid == cred->fsuid) {
 		kperm = key->perm >> 16;
@@ -56,6 +59,8 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
 		}
 	}
 
+use_other_perms:
+
 	/* otherwise use the least-significant 8-bits */
 	kperm = key->perm;
 
-- 
1.5.4.3

[PATCH 3/4] keys: skip keys from another user namespace

[Serge E. Hallyn]

When listing keys, do not return keys belonging to the
same uid in another user namespace.  Otherwise uid 500
in another user namespace will return keyrings called
uid.500 for another user namespace.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/keys/keyring.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index ed85157..3dba81c 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -539,6 +539,9 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
 				    &keyring_name_hash[bucket],
 				    type_data.link
 				    ) {
+			if (keyring->user->user_ns != current_user_ns())
+				continue;
+
 			if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
 				continue;
 
-- 
1.5.4.3

[PATCH 4/4] keys: make procfiles per-user-namespace

[Serge E. Hallyn]

Restrict the /proc/keys and /proc/key-users output to keys
belonging to the same user namespace as the reading task.

We may want to make this more complicated - so that any
keys in a user-namespace which is belongs to the reading
task are also shown.  But let's see if anyone wants that
first.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/keys/proc.c |   55 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index 7f508de..769f9bd 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -91,6 +91,28 @@ __initcall(key_proc_init);
  */
 #ifdef CONFIG_KEYS_DEBUG_PROC_KEYS
 
+static struct rb_node *__key_serial_next(struct rb_node *n)
+{
+	while (n) {
+		struct key *key = rb_entry(n, struct key, serial_node);
+		if (key->user->user_ns == current_user_ns())
+			break;
+		n = rb_next(n);
+	}
+	return n;
+}
+
+static struct rb_node *key_serial_next(struct rb_node *n)
+{
+	return __key_serial_next(rb_next(n));
+}
+
+static struct rb_node *key_serial_first(struct rb_root *r)
+{
+	struct rb_node *n = rb_first(r);
+	return __key_serial_next(n);
+}
+
 static int proc_keys_open(struct inode *inode, struct file *file)
 {
 	return seq_open(file, &proc_keys_ops);
@@ -104,10 +126,10 @@ static void *proc_keys_start(struct seq_file *p, loff_t *_pos)
 
 	spin_lock(&key_serial_lock);
 
-	_p = rb_first(&key_serial_tree);
+	_p = key_serial_first(&key_serial_tree);
 	while (pos > 0 && _p) {
 		pos--;
-		_p = rb_next(_p);
+		_p = key_serial_next(_p);
 	}
 
 	return _p;
@@ -117,7 +139,7 @@ static void *proc_keys_start(struct seq_file *p, loff_t *_pos)
 static void *proc_keys_next(struct seq_file *p, void *v, loff_t *_pos)
 {
 	(*_pos)++;
-	return rb_next((struct rb_node *) v);
+	return key_serial_next((struct rb_node *) v);
 
 }
 
@@ -203,6 +225,27 @@ static int proc_keys_show(struct seq_file *m, void *v)
 
 #endif /* CONFIG_KEYS_DEBUG_PROC_KEYS */
 
+static struct rb_node *__key_user_next(struct rb_node *n)
+{
+	while (n) {
+		struct key_user *user = rb_entry(n, struct key_user, node);
+		if (user->user_ns == current_user_ns())
+			break;
+		n = rb_next(n);
+	}
+	return n;
+}
+
+static struct rb_node *key_user_next(struct rb_node *n)
+{
+	return __key_user_next(rb_next(n));
+}
+
+static struct rb_node *key_user_first(struct rb_root *r)
+{
+	struct rb_node *n = rb_first(r);
+	return __key_user_next(n);
+}
 /*****************************************************************************/
 /*
  * implement "/proc/key-users" to provides a list of the key users
@@ -220,10 +263,10 @@ static void *proc_key_users_start(struct seq_file *p, loff_t *_pos)
 
 	spin_lock(&key_user_lock);
 
-	_p = rb_first(&key_user_tree);
+	_p = key_user_first(&key_user_tree);
 	while (pos > 0 && _p) {
 		pos--;
-		_p = rb_next(_p);
+		_p = key_user_next(_p);
 	}
 
 	return _p;
@@ -233,7 +276,7 @@ static void *proc_key_users_start(struct seq_file *p, loff_t *_pos)
 static void *proc_key_users_next(struct seq_file *p, void *v, loff_t *_pos)
 {
 	(*_pos)++;
-	return rb_next((struct rb_node *) v);
+	return key_user_next((struct rb_node *) v);
 
 }
 
-- 
1.5.4.3

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> Restrict the /proc/keys and /proc/key-users output to keys
> belonging to the same user namespace as the reading task.
> 
> We may want to make this more complicated - so that any
> keys in a user-namespace which is belongs to the reading
> task are also shown.  But let's see if anyone wants that
> first.

Hmmm...  I wonder if we can do better by making the file position indicate the
key ID rather than being a count of the number of keys read.  It might make
this cleaner.

David

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > Restrict the /proc/keys and /proc/key-users output to keys
> > belonging to the same user namespace as the reading task.
> > 
> > We may want to make this more complicated - so that any
> > keys in a user-namespace which is belongs to the reading
> > task are also shown.  But let's see if anyone wants that
> > first.
> 
> Hmmm...  I wonder if we can do better by making the file position indicate the
> key ID rather than being a count of the number of keys read.  It might make
> this cleaner.

file position?  as in the result of lseek(fd, 0, SEEK_CUR)?

I don't understand what you're suggesting.

-serge

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> > Hmmm...  I wonder if we can do better by making the file position indicate
> > the key ID rather than being a count of the number of keys read.  It might
> > make this cleaner.
> 
> file position?  as in the result of lseek(fd, 0, SEEK_CUR)?
> 
> I don't understand what you're suggesting.

Currently the file position on /proc/keys indicates the number of keys that
have been read.  It is incremented by 1 for each key read, irrespective of the
length of the line that was read for that key.

We could, instead, map file positions to key IDs, and skip any file positions
that don't actually map to an extant key.

David

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > > Hmmm...  I wonder if we can do better by making the file position indicate
> > > the key ID rather than being a count of the number of keys read.  It might
> > > make this cleaner.
> > 
> > file position?  as in the result of lseek(fd, 0, SEEK_CUR)?
> > 
> > I don't understand what you're suggesting.
> 
> Currently the file position on /proc/keys indicates the number of keys that
> have been read.  It is incremented by 1 for each key read, irrespective of the
> length of the line that was read for that key.
> 
> We could, instead, map file positions to key IDs, and skip any file positions
> that don't actually map to an extant key.

So you want users to be able to mmap the file and lseek to a particular
spot?  Is that bc you have users with so many keys that
	grep keyid /proc/keys
becomes slow?

Or am I still misunderstanding?

Meanwhile, do you have any objections to these 4 patches?  :)

thanks,
-serge

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> So you want users to be able to mmap the file and lseek to a particular
> spot?

mmap?  *blink*

> Meanwhile, do you have any objections to these 4 patches?  :)

Not really.  You can add my Acked-by.  It's just that mapping fpos to keyid
might simplify the code.

David

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > So you want users to be able to mmap the file and lseek to a particular
> > spot?
> 
> mmap?  *blink*

/me pretends he didn't say that

> > Meanwhile, do you have any objections to these 4 patches?  :)
> 
> Not really.  You can add my Acked-by.

Thanks.

> It's just that mapping fpos to keyid
> might simplify the code.

Ok i think i see what you mean.  I'll give it a shot.

thanks,
-serge

Re: [PATCH 4/4] keys: make procfiles per-user-namespace

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > Restrict the /proc/keys and /proc/key-users output to keys
> > belonging to the same user namespace as the reading task.
> > 
> > We may want to make this more complicated - so that any
> > keys in a user-namespace which is belongs to the reading
> > task are also shown.  But let's see if anyone wants that
> > first.
> 
> Hmmm...  I wonder if we can do better by making the file position indicate the
> key ID rather than being a count of the number of keys read.  It might make
> this cleaner.

Ok, what I came up with so far is the following.  The diffstat
would be far more impressive (in terms of - vs +) if I could
use key_lookup() for proc_keys_start(), but since I need to
return the next greatest key, it seems like I need to do my
own find_ge_key() function.

>From cf3961ed60d162bb2b1a3da231009733fd114546 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
Date: Thu, 26 Feb 2009 13:29:59 -0800
Subject: [PATCH 1/1] keys: /proc/keys: use keyid not numread as fpos

Just an experiment - previously the fpos used in
printing /proc/keys through seq_file interface
represented number of items read.  This patch
instead stores the key->serial in fpos.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 security/keys/proc.c |   63 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index 769f9bd..6132629 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -91,8 +91,9 @@ __initcall(key_proc_init);
  */
 #ifdef CONFIG_KEYS_DEBUG_PROC_KEYS
 
-static struct rb_node *__key_serial_next(struct rb_node *n)
+static struct rb_node *key_serial_next(struct rb_node *n)
 {
+	n = rb_next(n);
 	while (n) {
 		struct key *key = rb_entry(n, struct key, serial_node);
 		if (key->user->user_ns == current_user_ns())
@@ -102,44 +103,62 @@ static struct rb_node *__key_serial_next(struct rb_node *n)
 	return n;
 }
 
-static struct rb_node *key_serial_next(struct rb_node *n)
+static int proc_keys_open(struct inode *inode, struct file *file)
 {
-	return __key_serial_next(rb_next(n));
-}
+	return seq_open(file, &proc_keys_ops);
 
-static struct rb_node *key_serial_first(struct rb_root *r)
-{
-	struct rb_node *n = rb_first(r);
-	return __key_serial_next(n);
 }
 
-static int proc_keys_open(struct inode *inode, struct file *file)
+static struct key *find_ge_key(unsigned int id)
 {
-	return seq_open(file, &proc_keys_ops);
+	struct rb_node *n = key_serial_tree.rb_node;
+	struct key *minkey = NULL;
+
+	while (n) {
+		struct key *key = rb_entry(n, struct key, serial_node);
+		if (id < key->serial) {
+			if (!minkey || minkey->serial > key->serial)
+				minkey = key;
+			n = n->rb_left;
+		} else if (id > key->serial)
+			n = n->rb_right;
+		else {
+			minkey = key;
+			break;
+		}
+		key = NULL;
+	}
 
+	return minkey;
 }
 
 static void *proc_keys_start(struct seq_file *p, loff_t *_pos)
 {
-	struct rb_node *_p;
-	loff_t pos = *_pos;
+	struct key *key;
+	unsigned int pos = *_pos;
 
 	spin_lock(&key_serial_lock);
+	key = find_ge_key(pos);
+	if (!key)
+		return NULL;
+	*_pos = key->serial;
+	return &key->serial_node;
+}
 
-	_p = key_serial_first(&key_serial_tree);
-	while (pos > 0 && _p) {
-		pos--;
-		_p = key_serial_next(_p);
-	}
-
-	return _p;
-
+static inline unsigned int key_node_serial(struct rb_node *n)
+{
+	struct key *key = rb_entry(n, struct key, serial_node);
+	return key->serial;
 }
 
 static void *proc_keys_next(struct seq_file *p, void *v, loff_t *_pos)
 {
-	(*_pos)++;
-	return key_serial_next((struct rb_node *) v);
+	struct rb_node *n;
+
+	n = key_serial_next(v);
+	if (n)
+		*_pos = key_node_serial(n);
+	return n;
 
 }
 
-- 
1.5.4.3

[PATCH 0/4] keys: work correctly with user namespaces

[Serge E. Hallyn]

Hi James,

the following 4 patches apply on top of current security-testing-2.6 next
branch.  They implement strict separation of keys in different user
namespaces.  Do you mind sticking these in the security-testing tree?

With these patches I can run the keyutils-tests simultaneously in two
namespaces with no problems or failures.

thanks,
-serge

[PATCH 3/4] keys: skip keys from another user namespace

[Serge E. Hallyn]

When listing keys, do not return keys belonging to the
same uid in another user namespace.  Otherwise uid 500
in another user namespace will return keyrings called
uid.500 for another user namespace.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Acked-by: David Howells <dhowells@redhat.com>
---
 security/keys/keyring.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index ed85157..3dba81c 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -539,6 +539,9 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
 				    &keyring_name_hash[bucket],
 				    type_data.link
 				    ) {
+			if (keyring->user->user_ns != current_user_ns())
+				continue;
+
 			if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
 				continue;
 
-- 
1.5.4.3