From cea7daa3589d6b550546a8c8963599f7c1a3ae5c Mon Sep 17 00:00:00 2001 From: Toshiyuki Okajima Date: Fri, 30 Apr 2010 14:32:13 +0100 Subject: KEYS: find_keyring_by_name() can gain access to a freed keyring 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: [] 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:[] [] 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: [] ? get_empty_filp+0x70/0x12f [] ? do_filp_open+0x145/0x590 [] ? tlb_finish_mmu+0x2a/0x33 [] ? unmap_region+0xd3/0xe2 [] ? virt_to_head_page+0x9/0x2d [] ? alloc_fd+0x69/0x10e [] ? do_sys_open+0x56/0xfc [] ? 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 [] 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 /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 Signed-off-by: David Howells Tested-by: Toshiyuki Okajima Acked-by: Serge Hallyn Signed-off-by: James Morris --- security/keys/keyring.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'security/keys') diff --git a/security/keys/keyring.c b/security/keys/keyring.c index dd7cd0f8e13..0b27271c670 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() */ -- cgit v1.2.3 From f0641cba7729e5e14f82d2eedc398103f5fa31b1 Mon Sep 17 00:00:00 2001 From: David Howells Date: Fri, 30 Apr 2010 14:32:18 +0100 Subject: KEYS: Use RCU dereference wrappers in keyring key type code The keyring key type code should use RCU dereference wrappers, even when it holds the keyring's key semaphore. Reported-by: Vegard Nossum Signed-off-by: David Howells Acked-by: Serge Hallyn Signed-off-by: James Morris --- security/keys/keyring.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'security/keys') diff --git a/security/keys/keyring.c b/security/keys/keyring.c index 0b27271c670..1e4b0037935 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -20,6 +20,11 @@ #include #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); @@ -868,7 +869,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++) @@ -959,7 +960,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, @@ -991,7 +992,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); @@ -1025,7 +1028,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; -- cgit v1.2.3 From 896903c2f5f79f029388f033a00c3b813bc91201 Mon Sep 17 00:00:00 2001 From: David Howells Date: Fri, 30 Apr 2010 14:32:23 +0100 Subject: KEYS: call_sbin_request_key() must write lock keyrings before modifying them 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: [] key_type_lookup+0x1c/0x71 #1: (keyring_serialise_link_sem){+.+.+.}, at: [] __key_link+0x4d/0x3c5 stack backtrace: Pid: 2076, comm: keyctl Not tainted 2.6.34-rc6-cachefs #54 Call Trace: [] lockdep_rcu_dereference+0xaa/0xb2 [] ? __key_link+0x4d/0x3c5 [] __key_link+0x19e/0x3c5 [] ? __key_instantiate_and_link+0xb1/0xdc [] ? key_instantiate_and_link+0x42/0x5f [] call_sbin_request_key+0xe7/0x33b [] ? mutex_unlock+0x9/0xb [] ? __key_instantiate_and_link+0xb1/0xdc [] ? key_instantiate_and_link+0x42/0x5f [] ? request_key_auth_new+0x1c2/0x23c [] ? cache_alloc_debugcheck_after+0x108/0x173 [] ? request_key_and_link+0x146/0x300 [] ? kmem_cache_alloc+0xe1/0x118 [] request_key_and_link+0x28b/0x300 [] sys_request_key+0xf7/0x14a [] ? trace_hardirqs_on_caller+0x10c/0x130 [] ? trace_hardirqs_on_thunk+0x3a/0x3f [] system_call_fastpath+0x16/0x1b Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/request_key.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/keys') diff --git a/security/keys/request_key.c b/security/keys/request_key.c index d737cea5347..d8c1a6a0fb0 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; -- cgit v1.2.3