Fix an RCU warning in the reading of user keys:

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
security/keys/user_defined.c:202 invoked rcu_dereference_check() without protection!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 0
1 lock held by keyctl/3637:
 #0:  (&key->sem){+++++.}, at: [<ffffffff811a80ae>] keyctl_read_key+0x9c/0xcf

stack backtrace:
Pid: 3637, comm: keyctl Not tainted 2.6.34-rc5-cachefs #18
Call Trace:
 [<ffffffff81051f6c>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffff811aa55f>] user_read+0x47/0x91
 [<ffffffff811a80be>] keyctl_read_key+0xac/0xcf
 [<ffffffff811a8a06>] sys_keyctl+0x75/0xb7
 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/user_defined.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 7c687d5..e9aa079 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -199,7 +199,8 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
 	struct user_key_payload *upayload;
 	long ret;
 
-	upayload = rcu_dereference(key->payload.data);
+	upayload = rcu_dereference_protected(
+		key->payload.data, rwsem_is_locked(&((struct key *)key)->sem));
 	ret = upayload->datalen;
 
 	/* we can return the data as is */

--

[ message continues ]

From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>

find_keyring_by_name() can gain access to a keyring that has had its reference
count reduced to zero, and is thus ready to be freed.  This then allows the
dead keyring to be brought back into use whilst it is being destroyed.

The following timeline illustrates the process:

|(cleaner)                           (user)
|
| free_user(user)                    sys_keyctl()
|  |                                  |
|  key_put(user->session_keyring)     keyctl_get_keyring_ID()
|  ||	//=> keyring->usage = 0        |
|  |schedule_work(&key_cleanup_task)   lookup_user_key()
|  ||                                   |
|  kmem_cache_free(,user)               |
|  .                                    |[KEY_SPEC_USER_KEYRING]
|  .                                    install_user_keyrings()
|  .                                    ||
| key_cleanup() [<= worker_thread()]    ||
|  |                                    ||
|  [spin_lock(&key_serial_lock)]        |[mutex_lock(&key_user_keyr..mutex)]
|  |                                    ||
|  atomic_read() == 0                   ||
|  |{ rb_ease(&key->serial_node,) }     ||
|  |                                    ||
|  [spin_unlock(&key_serial_lock)]      |find_keyring_by_name()
|  |                                    |||
|  keyring_destroy(keyring)             ||[read_lock(&keyring_name_lock)]
|  ||                                   |||
|  |[write_lock(&keyring_name_lock)]    ||atomic_inc(&keyring->usage)
|  |.                                   ||| *** GET freeing keyring ***
|  |.                                   ||[read_unlock(&keyring_name_lock)]
|  ||                                   ||
|  |list_del()                          |[mutex_unlock(&key_user_k..mutex)]
|  ||                                   |
|  |[write_unlock(&keyring_name_lock)]  ** INVALID keyring is returned **
|  |                                    .
|  kmem_cache_free(,keyring)            .
|                        ...
[ message continues ]

(looks good, fwiw)

Acked-by: Serge Hallyn <serue@us.ibm.com>
--

[ message continues ]

Errors from construct_alloc_key() shouldn't just be ignored in the way they are
by construct_key_and_link().  The only error that can be ignored so is
EINPROGRESS as that is used to indicate that we've found a key and don't need
to construct one.

We don't, however, handle ENOMEM, EDQUOT or EACCES to indicate allocation
failures of one sort or another.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/request_key.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index d8c1a6a..ac49c8a 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -302,6 +302,7 @@ static int construct_alloc_key(struct key_type *type,
 	const struct cred *cred = current_cred();
 	struct key *key;
 	key_ref_t key_ref;
+	int ret;
 
 	kenter("%s,%s,,,", type->name, description);
 
@@ -337,14 +338,23 @@ static int construct_alloc_key(struct key_type *type,
 	kleave(" = 0 [%d]", key_serial(key));
 	return 0;
 
+	/* the key is now present - we tell the caller that we found it by
+	 * returning -EINPROGRESS  */
 key_already_present:
 	mutex_unlock(&key_construction_mutex);
+	ret = 0;
 	if (dest_keyring) {
-		__key_link(dest_keyring, key_ref_to_ptr(key_ref));
+		ret = __key_link(dest_keyring, key_ref_to_ptr(key_ref));
 		up_write(&dest_keyring->sem);
 	}
 	mutex_unlock(&user->cons_lock);
 	key_put(key);
+	if (ret < 0) {
+		key_ref_put(key_ref);
+		*_key = NULL;
+		kleave(" = %d [link]", ret);
+		return ret;
+	}
 	*_key = key = key_ref_to_ptr(key_ref);
 	kleave(" = -EINPROGRESS [%d]", key_serial(key));
 	return -EINPROGRESS;
@@ -390,6 +400,10 @@ static struct key *construct_key_and_link(struct key_type *type,
 			kdebug("cons failed");
 			goto construction_failed;
 		}
+	} else if (ret == -EINPROGRESS) {
+		ret = 0;
+	} else {
+		key = ERR_PTR(ret);
 	}
 
 ...
[ message continues ]

Do preallocation for __key_link() so that the various callers in request_key.c
can deal with any errors from this source before attempting to construct a key.
This allows them to assume that the actual linkage step is guaranteed to be
successful.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/internal.h    |   11 ++
 security/keys/key.c         |   45 +++++---
 security/keys/keyring.c     |  242 ++++++++++++++++++++++++++-----------------
 security/keys/request_key.c |   47 ++++++--
 4 files changed, 215 insertions(+), 130 deletions(-)

diff --git a/security/keys/internal.h b/security/keys/internal.h
index 24ba030..5d4402a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -87,7 +87,16 @@ extern wait_queue_head_t request_key_conswq;
 extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
-extern int __key_link(struct key *keyring, struct key *key);
+extern int __key_link_begin(struct key *keyring,
+			    const struct key_type *type,
+			    const char *description,
+			    struct keyring_list **_prealloc);
+extern int __key_link_check_live_key(struct key *keyring, struct key *key);
+extern void __key_link(struct key *keyring, struct key *key,
+		       struct keyring_list **_prealloc);
+extern void __key_link_end(struct key *keyring,
+			   struct key_type *type,
+			   struct keyring_list *prealloc);
 
 extern key_ref_t __keyring_search_one(key_ref_t keyring_ref,
 				      const struct key_type *type,
diff --git a/security/keys/key.c b/security/keys/key.c
index e50d264..4a5984c 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -398,7 +398,8 @@ static int __key_instantiate_and_link(struct key *key,
 				      const void *data,
 				      size_t datalen,
 				      struct key *keyring,
-				      struct key *authkey)
+				      struct key *authkey,
+				      struct keyring_list **_prealloc)
 {
 	int ret, awaken;
 
@@ -425,7 +426,7 @@ static int ...
[ message continues ]

keyring_serialise_link_sem is only needed for keyring->keyring links as it's
used to prevent cycle detection from being avoided by parallel keyring
additions.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 63385af..4dc2284 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -711,13 +711,14 @@ int __key_link(struct key *keyring, struct key *key)
 	if (keyring->type != &key_type_keyring)
 		goto error;
 
-	/* serialise link/link calls to prevent parallel calls causing a
-	 * cycle when applied to two keyring in opposite orders */
-	down_write(&keyring_serialise_link_sem);
-
-	/* check that we aren't going to create a cycle adding one keyring to
-	 * another */
+	/* do some special keyring->keyring link checks */
 	if (key->type == &key_type_keyring) {
+		/* serialise link/link calls to prevent parallel calls causing
+		 * a cycle when applied to two keyring in opposite orders */
+		down_write(&keyring_serialise_link_sem);
+
+		/* check that we aren't going to create a cycle adding one
+		 * keyring to another */
 		ret = keyring_detect_cycle(keyring, key);
 		if (ret < 0)
 			goto error2;
@@ -817,7 +818,8 @@ int __key_link(struct key *keyring, struct key *key)
 done:
 	ret = 0;
 error2:
-	up_write(&keyring_serialise_link_sem);
+	if (key->type == &key_type_keyring)
+		up_write(&keyring_serialise_link_sem);
 error:
 	return ret;
 

--

[ message continues ]

The keyring key type code should use RCU dereference wrappers, even when it
holds the keyring's key semaphore.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index d570824..63385af 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -20,6 +20,11 @@
 #include <linux/uaccess.h>
 #include "internal.h"
 
+#define rcu_dereference_locked_keyring(keyring)				\
+	(rcu_dereference_protected(					\
+		(keyring)->payload.subscriptions,			\
+		rwsem_is_locked((struct rw_semaphore *)&(keyring)->sem)))
+
 /*
  * when plumbing the depths of the key tree, this sets a hard limit set on how
  * deep we're willing to go
@@ -201,8 +206,7 @@ static long keyring_read(const struct key *keyring,
 	int loop, ret;
 
 	ret = 0;
-	klist = keyring->payload.subscriptions;
-
+	klist = rcu_dereference_locked_keyring(keyring);
 	if (klist) {
 		/* calculate how much data we could return */
 		qty = klist->nkeys * sizeof(key_serial_t);
@@ -720,8 +724,7 @@ int __key_link(struct key *keyring, struct key *key)
 	}
 
 	/* see if there's a matching key we can displace */
-	klist = keyring->payload.subscriptions;
-
+	klist = rcu_dereference_locked_keyring(keyring);
 	if (klist && klist->nkeys > 0) {
 		struct key_type *type = key->type;
 
@@ -765,8 +768,6 @@ int __key_link(struct key *keyring, struct key *key)
 	if (ret < 0)
 		goto error2;
 
-	klist = keyring->payload.subscriptions;
-
 	if (klist && klist->nkeys < klist->maxkeys) {
 		/* there's sufficient slack space to add directly */
 		atomic_inc(&key->usage);
@@ -867,7 +868,7 @@ int key_unlink(struct key *keyring, struct key *key)
 
 	down_write(&keyring->sem);
 
-	klist = keyring->payload.subscriptions;
+	klist = rcu_dereference_locked_keyring(keyring);
 	if (klist) {
 		/* search ...
[ message continues ]

Acked-by: Serge Hallyn <serue@us.ibm.com>


<shrug> does this mean that the
        klist = rcu_dereference_check(keyring->payload.subscriptions,
				      lockdep_is_held(&key_serial_lock));
in security/keys/gc.c:key_gc_keyring() should become a
rcu_dereference_protected() to avoid the rcu_dereference_raw() and for

Weird, this was a straight rcu_dereference in my tree (which would

huh, yeah, seems to have been there and unneeded since at least 2007.

-serge
--

[ message continues ]

Well spotted.  That's incorrect modification by commit
e7b0a61b7929632d36cf052d9e2820ef0a9c1bfe.

key_serial_lock is not a guarantor of the current keyring subscriptions not
changing as we read it.  We need to hold either the RCU read lock (so that the
what the pointer currently points to isn't trashed) or the keyring semaphore
(so that the keyring isn't changed under us).

The real error is not holding the RCU read lock (I'd assumed that the fact it
was holding a spinlock implied this - which I now know to be wrong).

David
--

[ message continues ]

heck it also serves to document it a bit, as looking at the fn
itself it's not clear that it is called under key->sem.

--

[ message continues ]

Give or take the banner comment on user_read() where it states this explicitly:

	/*
	 * read the key data
	 * - the key's semaphore is read-locked
	 */
	long user_read(const struct key *key, char __user *buffer, size_t buflen)

and Documentation/keys.txt where it also states this explicitly:

 (*) long (*read)(const struct key *key, char __user *buffer, size_t buflen);

     ...

     This method will be called with the key's semaphore read-locked. This will
     prevent the key's payload changing. It is not necessary to use RCU locking
     when accessing the key's payload. It is safe to sleep in this method, such

David

David
--

[ message continues ]

Okay... patches 1-4 have gone into -linus, along with 'KEYS: Fix RCU 
handling in key_gc_keyring()'

The should probably all be in -stable (see the most recent 'keys' patches 
in -linus).

Patches 5 & 6 are in:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

The final patch does not apply to my tree -- please rebase it.


- James
-- 
James Morris
<jmorris@namei.org>
--

[ message continues ]

Your tree is not up to date with respect to Linus's: you're missing the three
patches he pulled in yesterday.

David
--

[ message continues ]

Ok, I forgot to pull from Linus' tree before merging locally, looks fine 
now.

-- 
James Morris
<jmorris@namei.org>
--

[ message continues ]