[PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[David Howells]

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 */

[PATCH 2/7] KEYS: find_keyring_by_name() can gain access to a freed keyring

[David Howells]

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)            .
|                                       .
|                                       atomic_dec(&keyring->usage)
v                                         *** DESTROYED ***
TIME

If CONFIG_SLUB_DEBUG=y then we may see the following message generated:

	=============================================================================
	BUG key_jar: Poison overwritten
	-----------------------------------------------------------------------------

	INFO: 0xffff880197a7e200-0xffff880197a7e200. First byte 0x6a instead of 0x6b
	INFO: Allocated in key_alloc+0x10b/0x35f age=25 cpu=1 pid=5086
	INFO: Freed in key_cleanup+0xd0/0xd5 age=12 cpu=1 pid=10
	INFO: Slab 0xffffea000592cb90 objects=16 used=2 fp=0xffff880197a7e200 flags=0x200000000000c3
	INFO: Object 0xffff880197a7e200 @offset=512 fp=0xffff880197a7e300

	Bytes b4 0xffff880197a7e1f0:  5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
	  Object 0xffff880197a7e200:  6a 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b jkkkkkkkkkkkkkkk


Alternatively, we may see a system panic happen, such as:

	BUG: unable to handle kernel NULL pointer dereference at 0000000000000001
	IP: [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
	PGD 6b2b4067 PUD 6a80d067 PMD 0
	Oops: 0000 [#1] SMP
	last sysfs file: /sys/kernel/kexec_crash_loaded
	CPU 1
	...
	Pid: 31245, comm: su Not tainted 2.6.34-rc5-nofixed-nodebug #2 D2089/PRIMERGY
	RIP: 0010:[<ffffffff810e61a3>]  [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
	RSP: 0018:ffff88006af3bd98  EFLAGS: 00010002
	RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff88007d19900b
	RDX: 0000000100000000 RSI: 00000000000080d0 RDI: ffffffff81828430
	RBP: ffffffff81828430 R08: ffff88000a293750 R09: 0000000000000000
	R10: 0000000000000001 R11: 0000000000100000 R12: 00000000000080d0
	R13: 00000000000080d0 R14: 0000000000000296 R15: ffffffff810f20ce
	FS:  00007f97116bc700(0000) GS:ffff88000a280000(0000) knlGS:0000000000000000
	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	CR2: 0000000000000001 CR3: 000000006a91c000 CR4: 00000000000006e0
	DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
	DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
	Process su (pid: 31245, threadinfo ffff88006af3a000, task ffff8800374414c0)
	Stack:
	 0000000512e0958e 0000000000008000 ffff880037f8d180 0000000000000001
	 0000000000000000 0000000000008001 ffff88007d199000 ffffffff810f20ce
	 0000000000008000 ffff88006af3be48 0000000000000024 ffffffff810face3
	Call Trace:
	 [<ffffffff810f20ce>] ? get_empty_filp+0x70/0x12f
	 [<ffffffff810face3>] ? do_filp_open+0x145/0x590
	 [<ffffffff810ce208>] ? tlb_finish_mmu+0x2a/0x33
	 [<ffffffff810ce43c>] ? unmap_region+0xd3/0xe2
	 [<ffffffff810e4393>] ? virt_to_head_page+0x9/0x2d
	 [<ffffffff81103916>] ? alloc_fd+0x69/0x10e
	 [<ffffffff810ef4ed>] ? do_sys_open+0x56/0xfc
	 [<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
	Code: 0f 1f 44 00 00 49 89 c6 fa 66 0f 1f 44 00 00 65 4c 8b 04 25 60 e8 00 00 48 8b 45 00 49 01 c0 49 8b 18 48 85 db 74 0d 48 63 45 18 <48> 8b 04 03 49 89 00 eb 14 4c 89 f9 83 ca ff 44 89 e6 48 89 ef
	RIP  [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9


This problem is that find_keyring_by_name does not confirm that the keyring is
valid before accepting it.

Skipping keyrings that have been reduced to a zero count seems the way to go.
To this end, use atomic_inc_not_zero() to increment the usage count and skip
the candidate keyring if that returns false.

The following script _may_ cause the bug to happen, but there's no guarantee
as the window of opportunity is small:

	#!/bin/sh
	LOOP=100000
	USER=dummy_user
	/bin/su -c "exit;" $USER || { /usr/sbin/adduser -m $USER; add=1; }
	for ((i=0; i<LOOP; i++))
	do
		/bin/su -c "echo '$i' > /dev/null" $USER
	done
	(( add == 1 )) && /usr/sbin/userdel -r $USER
	exit

Note that the nominated user must not be in use.

An alternative way of testing this may be:

	for ((i=0; i<100000; i++))
	do
		keyctl session foo /bin/true || break
	done >&/dev/null

as that uses a keyring named "foo" rather than relying on the user and
user-session named keyrings.

Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
---

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

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8fa2df0..d570824 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -526,9 +526,8 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
 	struct key *keyring;
 	int bucket;
 
-	keyring = ERR_PTR(-EINVAL);
 	if (!name)
-		goto error;
+		return ERR_PTR(-EINVAL);
 
 	bucket = keyring_hash(name);
 
@@ -555,17 +554,18 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
 					   KEY_SEARCH) < 0)
 				continue;
 
-			/* we've got a match */
-			atomic_inc(&keyring->usage);
-			read_unlock(&keyring_name_lock);
-			goto error;
+			/* we've got a match but we might end up racing with
+			 * key_cleanup() if the keyring is currently 'dead'
+			 * (ie. it has a zero usage count) */
+			if (!atomic_inc_not_zero(&keyring->usage))
+				continue;
+			goto out;
 		}
 	}
 
-	read_unlock(&keyring_name_lock);
 	keyring = ERR_PTR(-ENOKEY);
-
- error:
+out:
+	read_unlock(&keyring_name_lock);
 	return keyring;
 
 } /* end find_keyring_by_name() */

Re: [PATCH 2/7] KEYS: find_keyring_by_name() can gain access to a freed keyring

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>

(looks good, fwiw)

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

[PATCH 3/7] KEYS: Use RCU dereference wrappers in keyring key type code

[David Howells]

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 the keyring for the key */
 		for (loop = 0; loop < klist->nkeys; loop++)
@@ -958,7 +959,7 @@ int keyring_clear(struct key *keyring)
 		/* detach the pointer block with the locks held */
 		down_write(&keyring->sem);
 
-		klist = keyring->payload.subscriptions;
+		klist = rcu_dereference_locked_keyring(keyring);
 		if (klist) {
 			/* adjust the quota */
 			key_payload_reserve(keyring,
@@ -990,7 +991,9 @@ EXPORT_SYMBOL(keyring_clear);
  */
 static void keyring_revoke(struct key *keyring)
 {
-	struct keyring_list *klist = keyring->payload.subscriptions;
+	struct keyring_list *klist;
+
+	klist = rcu_dereference_locked_keyring(keyring);
 
 	/* adjust the quota */
 	key_payload_reserve(keyring, 0);
@@ -1024,7 +1027,7 @@ void keyring_gc(struct key *keyring, time_t limit)
 
 	down_write(&keyring->sem);
 
-	klist = keyring->payload.subscriptions;
+	klist = rcu_dereference_locked_keyring(keyring);
 	if (!klist)
 		goto no_klist;
 

Re: [PATCH 3/7] KEYS: Use RCU dereference wrappers in keyring key type code

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> 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>

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
consistency?

> ---
> 
>  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);

Weird, this was a straight rcu_dereference in my tree (which would
still deserve a switch for the same reason as patch 1 of course)

>  	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;
> -

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

-serge

Re: [PATCH 3/7] KEYS: Use RCU dereference wrappers in keyring key type code

[David Howells]

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

> <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
> consistency?

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

[PATCH 4/7] KEYS: call_sbin_request_key() must write lock keyrings before modifying them

[David Howells]

call_sbin_request_key() creates a keyring and then attempts to insert a link to
the authorisation key into that keyring, but does so without holding a write
lock on the keyring semaphore.

It will normally get away with this because it hasn't told anyone that the
keyring exists yet.  The new keyring, however, has had its serial number
published, which means it can be accessed directly by that handle.

This was found by a previous patch that adds RCU lockdep checks to the code
that reads the keyring payload pointer, which includes a check that the keyring
semaphore is actually locked.

Without this patch, the following command:

	keyctl request2 user b a @s

will provoke the following lockdep warning is displayed in dmesg:

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

	other info that might help us debug this:


	rcu_scheduler_active = 1, debug_locks = 0
	2 locks held by keyctl/2076:
	 #0:  (key_types_sem){.+.+.+}, at: [<ffffffff811a5b29>] key_type_lookup+0x1c/0x71
	 #1:  (keyring_serialise_link_sem){+.+.+.}, at: [<ffffffff811a6d1e>] __key_link+0x4d/0x3c5

	stack backtrace:
	Pid: 2076, comm: keyctl Not tainted 2.6.34-rc6-cachefs #54
	Call Trace:
	 [<ffffffff81051fdc>] lockdep_rcu_dereference+0xaa/0xb2
	 [<ffffffff811a6d1e>] ? __key_link+0x4d/0x3c5
	 [<ffffffff811a6e6f>] __key_link+0x19e/0x3c5
	 [<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
	 [<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
	 [<ffffffff811aa0dc>] call_sbin_request_key+0xe7/0x33b
	 [<ffffffff8139376a>] ? mutex_unlock+0x9/0xb
	 [<ffffffff811a5952>] ? __key_instantiate_and_link+0xb1/0xdc
	 [<ffffffff811a59bf>] ? key_instantiate_and_link+0x42/0x5f
	 [<ffffffff811aa6fa>] ? request_key_auth_new+0x1c2/0x23c
	 [<ffffffff810aaf15>] ? cache_alloc_debugcheck_after+0x108/0x173
	 [<ffffffff811a9d00>] ? request_key_and_link+0x146/0x300
	 [<ffffffff810ac568>] ? kmem_cache_alloc+0xe1/0x118
	 [<ffffffff811a9e45>] request_key_and_link+0x28b/0x300
	 [<ffffffff811a89ac>] sys_request_key+0xf7/0x14a
	 [<ffffffff81052c0b>] ? trace_hardirqs_on_caller+0x10c/0x130
	 [<ffffffff81394fb9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
	 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

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

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

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index d737cea..d8c1a6a 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -94,7 +94,7 @@ static int call_sbin_request_key(struct key_construction *cons,
 	}
 
 	/* attach the auth key to the session keyring */
-	ret = __key_link(keyring, authkey);
+	ret = key_link(keyring, authkey);
 	if (ret < 0)
 		goto error_link;
 

[PATCH 5/7] KEYS: keyring_serialise_link_sem is only needed for keyring->keyring links

[David Howells]

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;
 

[PATCH 6/7] KEYS: Better handling of errors from construct_alloc_key()

[David Howells]

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);
 	}
 
 	key_put(dest_keyring);
@@ -422,6 +436,7 @@ struct key *request_key_and_link(struct key_type *type,
 	const struct cred *cred = current_cred();
 	struct key *key;
 	key_ref_t key_ref;
+	int ret;
 
 	kenter("%s,%s,%p,%zu,%p,%p,%lx",
 	       type->name, description, callout_info, callout_len, aux,
@@ -435,8 +450,13 @@ struct key *request_key_and_link(struct key_type *type,
 		key = key_ref_to_ptr(key_ref);
 		if (dest_keyring) {
 			construct_get_dest_keyring(&dest_keyring);
-			key_link(dest_keyring, key);
+			ret = key_link(dest_keyring, key);
 			key_put(dest_keyring);
+			if (ret < 0) {
+				key_put(key);
+				key = ERR_PTR(ret);
+				goto error;
+			}
 		}
 	} else if (PTR_ERR(key_ref) != -EAGAIN) {
 		key = ERR_CAST(key_ref);

[PATCH 7/7] KEYS: Do preallocation for __key_link()

[David Howells]

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 __key_instantiate_and_link(struct key *key,
 
 			/* and link it into the destination keyring */
 			if (keyring)
-				ret = __key_link(keyring, key);
+				__key_link(keyring, key, _prealloc);
 
 			/* disable the authorisation key */
 			if (authkey)
@@ -453,15 +454,21 @@ int key_instantiate_and_link(struct key *key,
 			     struct key *keyring,
 			     struct key *authkey)
 {
+	struct keyring_list *prealloc;
 	int ret;
 
-	if (keyring)
-		down_write(&keyring->sem);
+	if (keyring) {
+		ret = __key_link_begin(keyring, key->type, key->description,
+				       &prealloc);
+		if (ret < 0)
+			return ret;
+	}
 
-	ret = __key_instantiate_and_link(key, data, datalen, keyring, authkey);
+	ret = __key_instantiate_and_link(key, data, datalen, keyring, authkey,
+					 &prealloc);
 
 	if (keyring)
-		up_write(&keyring->sem);
+		__key_link_end(keyring, key->type, prealloc);
 
 	return ret;
 
@@ -478,8 +485,9 @@ int key_negate_and_link(struct key *key,
 			struct key *keyring,
 			struct key *authkey)
 {
+	struct keyring_list *prealloc;
 	struct timespec now;
-	int ret, awaken;
+	int ret, awaken, link_ret = 0;
 
 	key_check(key);
 	key_check(keyring);
@@ -488,7 +496,8 @@ int key_negate_and_link(struct key *key,
 	ret = -EBUSY;
 
 	if (keyring)
-		down_write(&keyring->sem);
+		link_ret = __key_link_begin(keyring, key->type,
+					    key->description, &prealloc);
 
 	mutex_lock(&key_construction_mutex);
 
@@ -508,8 +517,8 @@ int key_negate_and_link(struct key *key,
 		ret = 0;
 
 		/* and link it into the destination keyring */
-		if (keyring)
-			ret = __key_link(keyring, key);
+		if (keyring && link_ret == 0)
+			__key_link(keyring, key, &prealloc);
 
 		/* disable the authorisation key */
 		if (authkey)
@@ -519,13 +528,13 @@ int key_negate_and_link(struct key *key,
 	mutex_unlock(&key_construction_mutex);
 
 	if (keyring)
-		up_write(&keyring->sem);
+		__key_link_end(keyring, key->type, prealloc);
 
 	/* wake up anyone waiting for a key to be constructed */
 	if (awaken)
 		wake_up_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT);
 
-	return ret;
+	return ret == 0 ? link_ret : ret;
 
 } /* end key_negate_and_link() */
 
@@ -749,6 +758,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 			       key_perm_t perm,
 			       unsigned long flags)
 {
+	struct keyring_list *prealloc;
 	const struct cred *cred = current_cred();
 	struct key_type *ktype;
 	struct key *keyring, *key = NULL;
@@ -775,7 +785,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	if (keyring->type != &key_type_keyring)
 		goto error_2;
 
-	down_write(&keyring->sem);
+	ret = __key_link_begin(keyring, ktype, description, &prealloc);
+	if (ret < 0)
+		goto error_2;
 
 	/* if we're going to allocate a new key, we're going to have
 	 * to modify the keyring */
@@ -817,7 +829,8 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 
 	/* instantiate it and link it into the target keyring */
-	ret = __key_instantiate_and_link(key, payload, plen, keyring, NULL);
+	ret = __key_instantiate_and_link(key, payload, plen, keyring, NULL,
+					 &prealloc);
 	if (ret < 0) {
 		key_put(key);
 		key_ref = ERR_PTR(ret);
@@ -827,7 +840,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
 
  error_3:
-	up_write(&keyring->sem);
+	__key_link_end(keyring, ktype, prealloc);
  error_2:
 	key_type_put(ktype);
  error:
@@ -837,7 +850,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	/* we found a matching key, so we're going to try to update it
 	 * - we can drop the locks first as we have the key pinned
 	 */
-	up_write(&keyring->sem);
+	__key_link_end(keyring, ktype, prealloc);
 	key_type_put(ktype);
 
 	key_ref = __key_update(key_ref, payload, plen);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4dc2284..b2ce9a8 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -662,20 +662,6 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
 
 } /* end keyring_detect_cycle() */
 
-/*****************************************************************************/
-/*
- * dispose of a keyring list after the RCU grace period
- */
-static void keyring_link_rcu_disposal(struct rcu_head *rcu)
-{
-	struct keyring_list *klist =
-		container_of(rcu, struct keyring_list, rcu);
-
-	kfree(klist);
-
-} /* end keyring_link_rcu_disposal() */
-
-/*****************************************************************************/
 /*
  * dispose of a keyring list after the RCU grace period, freeing the unlinked
  * key
@@ -685,56 +671,51 @@ static void keyring_unlink_rcu_disposal(struct rcu_head *rcu)
 	struct keyring_list *klist =
 		container_of(rcu, struct keyring_list, rcu);
 
-	key_put(klist->keys[klist->delkey]);
+	if (klist->delkey != USHORT_MAX)
+		key_put(klist->keys[klist->delkey]);
 	kfree(klist);
+}
 
-} /* end keyring_unlink_rcu_disposal() */
-
-/*****************************************************************************/
 /*
- * link a key into to a keyring
- * - must be called with the keyring's semaphore write-locked
- * - discard already extant link to matching key if there is one
+ * preallocate memory so that a key can be linked into to a keyring
  */
-int __key_link(struct key *keyring, struct key *key)
+int __key_link_begin(struct key *keyring, const struct key_type *type,
+		     const char *description,
+		     struct keyring_list **_prealloc)
+	__acquires(&keyring->sem)
 {
 	struct keyring_list *klist, *nklist;
 	unsigned max;
 	size_t size;
 	int loop, ret;
 
-	ret = -EKEYREVOKED;
-	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
-		goto error;
+	kenter("%d,%s,%s,", key_serial(keyring), type->name, description);
 
-	ret = -ENOTDIR;
 	if (keyring->type != &key_type_keyring)
-		goto error;
+		return -ENOTDIR;
+
+	down_write(&keyring->sem);
+
+	ret = -EKEYREVOKED;
+	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
+		goto error_krsem;
 
-	/* 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 */
+	/* serialise link/link calls to prevent parallel calls causing a cycle
+	 * when linking two keyring in opposite orders */
+	if (type == &key_type_keyring)
 		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;
-	}
+	klist = rcu_dereference_locked_keyring(keyring);
 
 	/* see if there's a matching key we can displace */
-	klist = rcu_dereference_locked_keyring(keyring);
 	if (klist && klist->nkeys > 0) {
-		struct key_type *type = key->type;
-
 		for (loop = klist->nkeys - 1; loop >= 0; loop--) {
 			if (klist->keys[loop]->type == type &&
 			    strcmp(klist->keys[loop]->description,
-				   key->description) == 0
+				   description) == 0
 			    ) {
-				/* found a match - replace with new key */
+				/* found a match - we'll replace this one with
+				 * the new key */
 				size = sizeof(struct key *) * klist->maxkeys;
 				size += sizeof(*klist);
 				BUG_ON(size > PAGE_SIZE);
@@ -742,22 +723,10 @@ int __key_link(struct key *keyring, struct key *key)
 				ret = -ENOMEM;
 				nklist = kmemdup(klist, size, GFP_KERNEL);
 				if (!nklist)
-					goto error2;
-
-				/* replace matched key */
-				atomic_inc(&key->usage);
-				nklist->keys[loop] = key;
-
-				rcu_assign_pointer(
-					keyring->payload.subscriptions,
-					nklist);
-
-				/* dispose of the old keyring list and the
-				 * displaced key */
-				klist->delkey = loop;
-				call_rcu(&klist->rcu,
-					 keyring_unlink_rcu_disposal);
+					goto error_sem;
 
+				/* note replacement slot */
+				klist->delkey = nklist->delkey = loop;
 				goto done;
 			}
 		}
@@ -767,16 +736,11 @@ int __key_link(struct key *keyring, struct key *key)
 	ret = key_payload_reserve(keyring,
 				  keyring->datalen + KEYQUOTA_LINK_BYTES);
 	if (ret < 0)
-		goto error2;
+		goto error_sem;
 
 	if (klist && klist->nkeys < klist->maxkeys) {
-		/* there's sufficient slack space to add directly */
-		atomic_inc(&key->usage);
-
-		klist->keys[klist->nkeys] = key;
-		smp_wmb();
-		klist->nkeys++;
-		smp_wmb();
+		/* there's sufficient slack space to append directly */
+		nklist = NULL;
 	} else {
 		/* grow the key list */
 		max = 4;
@@ -784,71 +748,155 @@ int __key_link(struct key *keyring, struct key *key)
 			max += klist->maxkeys;
 
 		ret = -ENFILE;
-		if (max > 65535)
-			goto error3;
+		if (max > USHORT_MAX - 1)
+			goto error_quota;
 		size = sizeof(*klist) + sizeof(struct key *) * max;
 		if (size > PAGE_SIZE)
-			goto error3;
+			goto error_quota;
 
 		ret = -ENOMEM;
 		nklist = kmalloc(size, GFP_KERNEL);
 		if (!nklist)
-			goto error3;
-		nklist->maxkeys = max;
-		nklist->nkeys = 0;
+			goto error_quota;
 
+		nklist->maxkeys = max;
 		if (klist) {
-			nklist->nkeys = klist->nkeys;
-			memcpy(nklist->keys,
-			       klist->keys,
+			memcpy(nklist->keys, klist->keys,
 			       sizeof(struct key *) * klist->nkeys);
+			nklist->delkey = klist->nkeys;
+			nklist->nkeys = klist->nkeys + 1;
+			klist->delkey = USHORT_MAX;
+		} else {
+			nklist->nkeys = 1;
+			nklist->delkey = 0;
 		}
 
 		/* add the key into the new space */
-		atomic_inc(&key->usage);
-		nklist->keys[nklist->nkeys++] = key;
-
-		rcu_assign_pointer(keyring->payload.subscriptions, nklist);
-
-		/* dispose of the old keyring list */
-		if (klist)
-			call_rcu(&klist->rcu, keyring_link_rcu_disposal);
+		nklist->keys[nklist->delkey] = NULL;
 	}
 
 done:
-	ret = 0;
-error2:
-	if (key->type == &key_type_keyring)
-		up_write(&keyring_serialise_link_sem);
-error:
-	return ret;
+	*_prealloc = nklist;
+	kleave(" = 0");
+	return 0;
 
-error3:
+error_quota:
 	/* undo the quota changes */
 	key_payload_reserve(keyring,
 			    keyring->datalen - KEYQUOTA_LINK_BYTES);
-	goto error2;
+error_sem:
+	if (type == &key_type_keyring)
+		up_write(&keyring_serialise_link_sem);
+error_krsem:
+	up_write(&keyring->sem);
+	kleave(" = %d", ret);
+	return ret;
+}
 
-} /* end __key_link() */
+/*
+ * check already instantiated keys aren't going to be a problem
+ * - the caller must have called __key_link_begin()
+ * - don't need to call this for keys that were created since __key_link_begin()
+ *   was called
+ */
+int __key_link_check_live_key(struct key *keyring, struct key *key)
+{
+	if (key->type == &key_type_keyring)
+		/* check that we aren't going to create a cycle by linking one
+		 * keyring to another */
+		return keyring_detect_cycle(keyring, key);
+	return 0;
+}
+
+/*
+ * link a key into to a keyring
+ * - must be called with __key_link_begin() having being called
+ * - discard already extant link to matching key if there is one
+ */
+void __key_link(struct key *keyring, struct key *key,
+		struct keyring_list **_prealloc)
+{
+	struct keyring_list *klist, *nklist;
+
+	nklist = *_prealloc;
+	*_prealloc = NULL;
+
+	kenter("%d,%d,%p", keyring->serial, key->serial, nklist);
+
+	klist = rcu_dereference_protected(keyring->payload.subscriptions,
+					  rwsem_is_locked(&keyring->sem));
+
+	atomic_inc(&key->usage);
+
+	/* there's a matching key we can displace or an empty slot in a newly
+	 * allocated list we can fill */
+	if (nklist) {
+		kdebug("replace %hu/%hu/%hu",
+		       nklist->delkey, nklist->nkeys, nklist->maxkeys);
+
+		nklist->keys[nklist->delkey] = key;
+
+		rcu_assign_pointer(keyring->payload.subscriptions, nklist);
+
+		/* dispose of the old keyring list and, if there was one, the
+		 * displaced key */
+		if (klist) {
+			kdebug("dispose %hu/%hu/%hu",
+			       klist->delkey, klist->nkeys, klist->maxkeys);
+			call_rcu(&klist->rcu, keyring_unlink_rcu_disposal);
+		}
+	} else {
+		/* there's sufficient slack space to append directly */
+		klist->keys[klist->nkeys] = key;
+		smp_wmb();
+		klist->nkeys++;
+	}
+}
+
+/*
+ * finish linking a key into to a keyring
+ * - must be called with __key_link_begin() having being called
+ */
+void __key_link_end(struct key *keyring, struct key_type *type,
+		    struct keyring_list *prealloc)
+	__releases(&keyring->sem)
+{
+	BUG_ON(type == NULL);
+	BUG_ON(type->name == NULL);
+	kenter("%d,%s,%p", keyring->serial, type->name, prealloc);
+
+	if (type == &key_type_keyring)
+		up_write(&keyring_serialise_link_sem);
+
+	if (prealloc) {
+		kfree(prealloc);
+		key_payload_reserve(keyring,
+				    keyring->datalen - KEYQUOTA_LINK_BYTES);
+	}
+	up_write(&keyring->sem);
+}
 
-/*****************************************************************************/
 /*
  * link a key to a keyring
  */
 int key_link(struct key *keyring, struct key *key)
 {
+	struct keyring_list *prealloc;
 	int ret;
 
 	key_check(keyring);
 	key_check(key);
 
-	down_write(&keyring->sem);
-	ret = __key_link(keyring, key);
-	up_write(&keyring->sem);
+	ret = __key_link_begin(keyring, key->type, key->description, &prealloc);
+	if (ret == 0) {
+		ret = __key_link_check_live_key(keyring, key);
+		if (ret == 0)
+			__key_link(keyring, key, &prealloc);
+		__key_link_end(keyring, key->type, prealloc);
+	}
 
 	return ret;
-
-} /* end key_link() */
+}
 
 EXPORT_SYMBOL(key_link);
 
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index ac49c8a..f656e9c 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -299,6 +299,7 @@ static int construct_alloc_key(struct key_type *type,
 			       struct key_user *user,
 			       struct key **_key)
 {
+	struct keyring_list *prealloc;
 	const struct cred *cred = current_cred();
 	struct key *key;
 	key_ref_t key_ref;
@@ -306,6 +307,7 @@ static int construct_alloc_key(struct key_type *type,
 
 	kenter("%s,%s,,,", type->name, description);
 
+	*_key = NULL;
 	mutex_lock(&user->cons_lock);
 
 	key = key_alloc(type, description, cred->fsuid, cred->fsgid, cred,
@@ -315,8 +317,12 @@ static int construct_alloc_key(struct key_type *type,
 
 	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
 
-	if (dest_keyring)
-		down_write(&dest_keyring->sem);
+	if (dest_keyring) {
+		ret = __key_link_begin(dest_keyring, type, description,
+				       &prealloc);
+		if (ret < 0)
+			goto link_prealloc_failed;
+	}
 
 	/* attach the key to the destination keyring under lock, but we do need
 	 * to do another check just in case someone beat us to it whilst we
@@ -328,11 +334,11 @@ static int construct_alloc_key(struct key_type *type,
 		goto key_already_present;
 
 	if (dest_keyring)
-		__key_link(dest_keyring, key);
+		__key_link(dest_keyring, key, &prealloc);
 
 	mutex_unlock(&key_construction_mutex);
 	if (dest_keyring)
-		up_write(&dest_keyring->sem);
+		__key_link_end(dest_keyring, type, prealloc);
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
 	kleave(" = 0 [%d]", key_serial(key));
@@ -341,27 +347,36 @@ static int construct_alloc_key(struct key_type *type,
 	/* the key is now present - we tell the caller that we found it by
 	 * returning -EINPROGRESS  */
 key_already_present:
+	key_put(key);
 	mutex_unlock(&key_construction_mutex);
-	ret = 0;
+	key = key_ref_to_ptr(key_ref);
 	if (dest_keyring) {
-		ret = __key_link(dest_keyring, key_ref_to_ptr(key_ref));
-		up_write(&dest_keyring->sem);
+		ret = __key_link_check_live_key(dest_keyring, key);
+		if (ret == 0)
+			__key_link(dest_keyring, key, &prealloc);
+		__key_link_end(dest_keyring, type, prealloc);
+		if (ret < 0)
+			goto link_check_failed;
 	}
 	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);
+	*_key = key;
 	kleave(" = -EINPROGRESS [%d]", key_serial(key));
 	return -EINPROGRESS;
 
+link_check_failed:
+	mutex_unlock(&user->cons_lock);
+	key_put(key);
+	kleave(" = %d [linkcheck]", ret);
+	return ret;
+
+link_prealloc_failed:
+	up_write(&dest_keyring->sem);
+	mutex_unlock(&user->cons_lock);
+	kleave(" = %d [prelink]", ret);
+	return ret;
+
 alloc_failed:
 	mutex_unlock(&user->cons_lock);
-	*_key = NULL;
 	kleave(" = %ld", PTR_ERR(key));
 	return PTR_ERR(key);
 }

Re: [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> 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>

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.

Acked-by: Serge Hallyn <serue@us.ibm.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 */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[David Howells]

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

> 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.

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
     as might happen when the userspace buffer is accessed.

:-)

David

David

Re: [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[James Morris]

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>

Re: [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[David Howells]

James Morris <jmorris@namei.org> wrote:

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

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

David

Re: [PATCH 1/7] KEYS: Fix an RCU warning in the reading of user keys

[James Morris]

On Thu, 6 May 2010, David Howells wrote:

> James Morris <jmorris@namei.org> wrote:
> 
> > The final patch does not apply to my tree -- please rebase it.
> 
> Your tree is not up to date with respect to Linus's: you're missing the three
> patches he pulled in yesterday.

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

-- 
James Morris
<jmorris@namei.org>