* [PATCH 1/1][BUG][IMPORTANT] KEYRINGS: find_keyring_by_name() can gain the freed keyring
@ 2010-04-22 7:37 Toshiyuki Okajima
2010-04-22 10:16 ` David Howells
0 siblings, 1 reply; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-22 7:37 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, security, linux-security-module, linux-kernel
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
With linux-2.6.34-rc5, find_keyring_by_name() can gain the keyring which has
been already freed. And then, its space (which is gained by
find_keyring_by_name()) is broken by accessing the freed keyring as the
available keyring.
This problem is serious because it may trigger the user data destructions.
[Figure Description](Example)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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)]
| | ||
| 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)] ||*** GET freeing keyring ***
| |. ||[read_unlock(&keyring_name_lock)]
| || |||
| |list_del() |||
| || |||
| |[write_unlock(&keyring_name_lock)] |||
| | |||
| kmem_cache_free(,keyring) |||
| ||atomic_inc(&keyring->usage)
| || ***DESTROYED***
| |[mutex_unlock(&key_user_k..mutex)]
v |
TIME ** INVALID keyring is returned **
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
If CONFIG_SLUB_DEBUG_ON is configured, we may see the following message in
dmesg:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
=============================================================================
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
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Otherwise, such a system panic may happen:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
<1>BUG: unable to handle kernel NULL pointer dereference at 0000000000000001
<1>IP: [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
<4>PGD 6b2b4067 PUD 6a80d067 PMD 0
<0>Oops: 0000 [#1] SMP
<0>last sysfs file: /sys/kernel/kexec_crash_loaded
<4>CPU 1
...
<4>Pid: 31245, comm: su Not tainted 2.6.34-rc5-nofixed-nodebug #2 D2089/PRIMERGY
<4>RIP: 0010:[<ffffffff810e61a3>] [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
<4>RSP: 0018:ffff88006af3bd98 EFLAGS: 00010002
<4>RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff88007d19900b
<4>RDX: 0000000100000000 RSI: 00000000000080d0 RDI: ffffffff81828430
<4>RBP: ffffffff81828430 R08: ffff88000a293750 R09: 0000000000000000
<4>R10: 0000000000000001 R11: 0000000000100000 R12: 00000000000080d0
<4>R13: 00000000000080d0 R14: 0000000000000296 R15: ffffffff810f20ce
<4>FS: 00007f97116bc700(0000) GS:ffff88000a280000(0000) knlGS:0000000000000000
<4>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>CR2: 0000000000000001 CR3: 000000006a91c000 CR4: 00000000000006e0
<4>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4>DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
<4>Process su (pid: 31245, threadinfo ffff88006af3a000, task ffff8800374414c0)
<0>Stack:
<4> 0000000512e0958e 0000000000008000 ffff880037f8d180 0000000000000001
<4><0> 0000000000000000 0000000000008001 ffff88007d199000 ffffffff810f20ce
<4><0> 0000000000008000 ffff88006af3be48 0000000000000024 ffffffff810face3
<0>Call Trace:
<4> [<ffffffff810f20ce>] ? get_empty_filp+0x70/0x12f
<4> [<ffffffff810face3>] ? do_filp_open+0x145/0x590
<4> [<ffffffff810ce208>] ? tlb_finish_mmu+0x2a/0x33
<4> [<ffffffff810ce43c>] ? unmap_region+0xd3/0xe2
<4> [<ffffffff810e4393>] ? virt_to_head_page+0x9/0x2d
<4> [<ffffffff81103916>] ? alloc_fd+0x69/0x10e
<4> [<ffffffff810ef4ed>] ? do_sys_open+0x56/0xfc
<4> [<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
<0>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
<1>RIP [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
This root cause is not to confirm the keyring is valid.
So, adding its validate confirmation with spin_lock(&key_serial_lock)
can fix this problem.
[Figure Description]
Case 1)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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)]
| | ||
| rb_ease(&key->serial_node,) ||
| | ||
| [spin_unlock(&key_serial_lock)] |find_keyring_by_name()
| | |||
| keyring_destroy(keyring) ||[read_lock(&keyring_name_lock)]
| || |||
| || ||[spin_lock(&key_serial_lock)]
| || |||
| || ||*** THIS keyring is freeing ***
| |[write_lock(&keyring_name_lock)] ||| So, returns with -ENOKEY
| |. ||[spin_unlock(&key_serial_lock)]
| |. |||
| |. ||[read_unlock(&keyring_name_lock)]
| || ||
| |list_del() |[mutex_unlock(&key_user_k..mutex)]
| || |
| |[write_unlock(&keyring_name_lock)] ** Keyring is not found. **
| |
| kmem_cache_free(,keyring)
v
TIME
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Case 2)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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()
| || |[KEY_SPEC_USER_KEYRING]
| kmem_cache_free(,user) install_user_keyrings()
| . ||
| . |[mutex_lock(&key_user_keyr..mutex)]
| . ||
| key_cleanup() [<= worker_thread()] |find_keyring_by_name()
| | |||
| | ||[read_lock(&keyring_name_lock)]
| | |||
| | ||[spin_lock(&key_serial_lock)]
| [spin_lock(&key_serial_lock)] |||
| . ||*** GET this keyring ***
| . |||
| . ||atomic_inc(&keyring->usage)
| . |||
| . ||[spin_unlock(&keyring_name_lock)]
| | |||
| *** NOT erase because usage>0 *** ||[read_unlock(&keyring_name_lock)]
| | ||
| [spin_unlock(&key_serial_lock)] |[mutex_unlock(&key_user_k..mutex)]
v |
TIME ** VALID keyring is returned **
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
---
security/keys/key.c | 1 +
security/keys/keyring.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/security/keys/key.c b/security/keys/key.c
index e50d264..3728bc9 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -559,6 +559,7 @@ static void key_cleanup(struct work_struct *work)
/* we found a dead key - once we've removed it from the tree, we can
* drop the lock */
rb_erase(&key->serial_node, &key_serial_tree);
+ RB_CLEAR_NODE(&key->serial_node); //this can enable RB_EMPTY_NODE()
spin_unlock(&key_serial_lock);
key_check(key);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e814d21..72e2b74 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -555,13 +555,23 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check)
KEY_SEARCH) < 0)
continue;
- /* we've got a match */
+ /* we've got a match but must confirm this keyring
+ * hasn't been yet removed from rb-tree */
+ spin_lock(&key_serial_lock);
+ if (RB_EMPTY_NODE(&keyring->serial_node)) {
+ /* this keyring is now freeing */
+ spin_unlock(&key_serial_lock);
+ goto not_found;
+ }
+ /* we let key_cleanup() not release this keyring */
atomic_inc(&keyring->usage);
+ spin_unlock(&key_serial_lock);
read_unlock(&keyring_name_lock);
goto error;
}
}
+not_found:
read_unlock(&keyring_name_lock);
keyring = ERR_PTR(-ENOKEY);
--
1.5.5.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][IMPORTANT] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-22 7:37 [PATCH 1/1][BUG][IMPORTANT] KEYRINGS: find_keyring_by_name() can gain the freed keyring Toshiyuki Okajima
@ 2010-04-22 10:16 ` David Howells
2010-04-23 10:45 ` Toshiyuki Okajima
0 siblings, 1 reply; 13+ messages in thread
From: David Howells @ 2010-04-22 10:16 UTC (permalink / raw)
To: Toshiyuki Okajima
Cc: dhowells, keyrings, security, linux-security-module, linux-kernel
Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
> With linux-2.6.34-rc5, find_keyring_by_name() can gain the keyring which has
> been already freed. And then, its space (which is gained by
> find_keyring_by_name()) is broken by accessing the freed keyring as the
> available keyring.
Good catch!
I'm not sure this is the best solution, though.
The alternative is just to ignore keys that have a zero usage count.
David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][IMPORTANT] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-22 10:16 ` David Howells
@ 2010-04-23 10:45 ` Toshiyuki Okajima
2010-04-23 10:51 ` [PATCH 1/1][BUG][TAKE2] " Toshiyuki Okajima
2010-04-23 11:33 ` David Howells
0 siblings, 2 replies; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-23 10:45 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, security, linux-security-module, linux-kernel
Hi.
Thanks for your comments.
David Howells wrote:
> Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
>
>> With linux-2.6.34-rc5, find_keyring_by_name() can gain the keyring which has
>> been already freed. And then, its space (which is gained by
>> find_keyring_by_name()) is broken by accessing the freed keyring as the
>> available keyring.
>
> Good catch!
>
> I'm not sure this is the best solution, though.
This means that the keyring of which the count became 0 should not usually
be used again?
If yes, I think so, too.
>
> The alternative is just to ignore keys that have a zero usage count.
OK. I applied your suggestion and I remade the patch.
Then I confirmed that the system with new fix could continue to work while
I was executing my reproducer and your reproducer.
[your reproducer]
> for ((i=0; i<100000; i++)); do keyctl session wibble /bin/true || break; done
So, I think my new patch is also fixed.
Please check it.(new patch is attached into the following mail.)
Thanks,
Toshiyuki Okajima
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 10:45 ` Toshiyuki Okajima
@ 2010-04-23 10:51 ` Toshiyuki Okajima
2010-04-23 11:33 ` David Howells
1 sibling, 0 replies; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-23 10:51 UTC (permalink / raw)
To: David Howells
Cc: keyrings, security, linux-security-module, linux-kernel,
Toshiyuki Okajima
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
With linux-2.6.34-rc5, find_keyring_by_name() can gain the keyring which has
been already freed. And then, its space (which is gained by
find_keyring_by_name()) is broken by accessing the freed keyring as the
available keyring.
This problem is serious because it may trigger the user data destructions.
[Figure Description](Example)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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_ON is configured, we may see the following message in
dmesg:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
=============================================================================
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
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Otherwise, such a system panic may happen:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
<1>BUG: unable to handle kernel NULL pointer dereference at 0000000000000001
<1>IP: [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
<4>PGD 6b2b4067 PUD 6a80d067 PMD 0
<0>Oops: 0000 [#1] SMP
<0>last sysfs file: /sys/kernel/kexec_crash_loaded
<4>CPU 1
...
<4>Pid: 31245, comm: su Not tainted 2.6.34-rc5-nofixed-nodebug #2 D2089/PRIMERGY
<4>RIP: 0010:[<ffffffff810e61a3>] [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
<4>RSP: 0018:ffff88006af3bd98 EFLAGS: 00010002
<4>RAX: 0000000000000000 RBX: 0000000000000001 RCX: ffff88007d19900b
<4>RDX: 0000000100000000 RSI: 00000000000080d0 RDI: ffffffff81828430
<4>RBP: ffffffff81828430 R08: ffff88000a293750 R09: 0000000000000000
<4>R10: 0000000000000001 R11: 0000000000100000 R12: 00000000000080d0
<4>R13: 00000000000080d0 R14: 0000000000000296 R15: ffffffff810f20ce
<4>FS: 00007f97116bc700(0000) GS:ffff88000a280000(0000) knlGS:0000000000000000
<4>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>CR2: 0000000000000001 CR3: 000000006a91c000 CR4: 00000000000006e0
<4>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4>DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
<4>Process su (pid: 31245, threadinfo ffff88006af3a000, task ffff8800374414c0)
<0>Stack:
<4> 0000000512e0958e 0000000000008000 ffff880037f8d180 0000000000000001
<4><0> 0000000000000000 0000000000008001 ffff88007d199000 ffffffff810f20ce
<4><0> 0000000000008000 ffff88006af3be48 0000000000000024 ffffffff810face3
<0>Call Trace:
<4> [<ffffffff810f20ce>] ? get_empty_filp+0x70/0x12f
<4> [<ffffffff810face3>] ? do_filp_open+0x145/0x590
<4> [<ffffffff810ce208>] ? tlb_finish_mmu+0x2a/0x33
<4> [<ffffffff810ce43c>] ? unmap_region+0xd3/0xe2
<4> [<ffffffff810e4393>] ? virt_to_head_page+0x9/0x2d
<4> [<ffffffff81103916>] ? alloc_fd+0x69/0x10e
<4> [<ffffffff810ef4ed>] ? do_sys_open+0x56/0xfc
<4> [<ffffffff81008a02>] ? system_call_fastpath+0x16/0x1b
<0>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
<1>RIP [<ffffffff810e61a3>] kmem_cache_alloc+0x5b/0xe9
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
This root cause is not to confirm the keyring is valid.
So, adding its validate confirmation with spin_lock(&key_serial_lock)
can fix this problem.
[Figure Description]
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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()] |[mutex_lock(&key_user_keyr..mutex)]
| | ||
| | |find_keyring_by_name()
| | |||
| | ||[read_lock(&keyring_name_lock)]
| | |||
| | ||atomic_inc_return(&ke..->usage)==1
| | ||| Freeing! will return with -ENOKEY
| | ||[spin_lock(&key_serial_lock)]
| | |||
| | ||atomic_set(&keyring->usage, 0)
| | ||| prevent from not starting freeing
| [spin_lock(&key_serial_lock)] ||| let key_cleanup() release this
| . ||[spin_unlock(&key_serial_lock)]
| | |||
| atomic_read(&keyring->usage) == 0 ||[read_unlock(&keyring_name_lock)]
| { rb_ease(&key->serial_node,) } ||
| | |[mutex_unlock(&key_user_k..mutex)]
| [spin_unlock(&key_serial_lock)] |
| | ** Keyring is not found. **
| keyring_destroy(keyring)
| ||
| |[write_lock(&keyring_name_lock)]
| ||
| |list_del()
| ||
| |[write_unlock(&keyring_name_lock)]
| |
| kmem_cache_free(,keyring)
v
TIME
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Cc: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e814d21..ec16456 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -555,13 +555,22 @@ 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);
+ /* we've got a match but must confirm this key
+ * is still valid */
+ if (atomic_inc_return(&keyring->usage) == 1) {
+ spin_lock(&key_serial_lock);
+ /* If this keyring is not still started freeing,
+ * we must let key_cleanup() release this */
+ atomic_set(&keyring->usage, 0);
+ spin_unlock(&key_serial_lock);
+ goto not_found;
+ }
read_unlock(&keyring_name_lock);
goto error;
}
}
+not_found:
read_unlock(&keyring_name_lock);
keyring = ERR_PTR(-ENOKEY);
--
1.5.5.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 10:45 ` Toshiyuki Okajima
2010-04-23 10:51 ` [PATCH 1/1][BUG][TAKE2] " Toshiyuki Okajima
@ 2010-04-23 11:33 ` David Howells
2010-04-23 15:23 ` Toshiyuki Okajima
` (2 more replies)
1 sibling, 3 replies; 13+ messages in thread
From: David Howells @ 2010-04-23 11:33 UTC (permalink / raw)
To: Toshiyuki Okajima
Cc: dhowells, keyrings, security, linux-security-module, linux-kernel
Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
> - /* we've got a match */
> - atomic_inc(&keyring->usage);
> + /* we've got a match but must confirm this key
> + * is still valid */
> + if (atomic_inc_return(&keyring->usage) == 1) {
> + spin_lock(&key_serial_lock);
> + /* If this keyring is not still started freeing,
> + * we must let key_cleanup() release this */
> + atomic_set(&keyring->usage, 0);
> + spin_unlock(&key_serial_lock);
> + goto not_found;
> + }
Better still, atomic_inc_not_zero(). How about the attached patch?
David
---
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Subject: [PATCH] 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)]
| | ||
| 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)] ||*** GET freeing keyring ***
| |. ||[read_unlock(&keyring_name_lock)]
| || |||
| |list_del() |||
| || |||
| |[write_unlock(&keyring_name_lock)] |||
| | |||
| kmem_cache_free(,keyring) |||
| ||atomic_inc(&keyring->usage)
| || ***DESTROYED***
| |[mutex_unlock(&key_user_k..mutex)]
v |
TIME ** INVALID keyring is returned **
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>
---
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() */
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 11:33 ` David Howells
@ 2010-04-23 15:23 ` Toshiyuki Okajima
2010-04-23 15:52 ` David Howells
2010-04-26 10:57 ` David Howells
2 siblings, 0 replies; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-23 15:23 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, security, linux-security-module, linux-kernel
On Fri, 23 Apr 2010 12:33:57 +0100
David Howells <dhowells@redhat.com> wrote:
> Better still, atomic_inc_not_zero(). How about the attached patch?
Your fix looks good to me. But, if usage count of the keyring is 0,
I think it better to return -ENOKEY immediately.
Like this.
> + /* 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;
=> break;
And my previous figure description(in first patch) was a bit wrong.
Please replace it with my new one:
[Figure Description](Example)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|(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
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Best Regards,
Toshiyuki Okajima
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 11:33 ` David Howells
2010-04-23 15:23 ` Toshiyuki Okajima
@ 2010-04-23 15:52 ` David Howells
2010-04-24 0:32 ` 岡嶋 寿行
` (2 more replies)
2010-04-26 10:57 ` David Howells
2 siblings, 3 replies; 13+ messages in thread
From: David Howells @ 2010-04-23 15:52 UTC (permalink / raw)
To: Toshiyuki Okajima
Cc: dhowells, keyrings, security, linux-security-module, linux-kernel
Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
> > Better still, atomic_inc_not_zero(). How about the attached patch?
> Your fix looks good to me. But, if usage count of the keyring is 0,
> I think it better to return -ENOKEY immediately.
The problem with that is that someone else may have created a keyring with the
same name that you can't then reach until the dead keyring is deleted.
David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 15:52 ` David Howells
@ 2010-04-24 0:32 ` 岡嶋 寿行
2010-04-26 14:22 ` Toshiyuki Okajima
2010-04-26 14:47 ` David Howells
2 siblings, 0 replies; 13+ messages in thread
From: 岡嶋 寿行 @ 2010-04-24 0:32 UTC (permalink / raw)
To: David Howells
Cc: Toshiyuki Okajima, dhowells, keyrings, security,
linux-security-module, linux-kernel
> Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
>
>> > Better still, atomic_inc_not_zero(). How about the attached patch?
>> Your fix looks good to me. But, if usage count of the keyring is 0,
>> I think it better to return -ENOKEY immediately.
>
> The problem with that is that someone else may have created a keyring with
> the
> same name that you can't then reach until the dead keyring is deleted.
OK. I understand.
---
Once find_keyring_by_name() returns -ENOKEY, the new user creates a
new key. So, both the deleting key and the new key may exist.
Therefore at next find_keyring_by_name() call, we should find the new
key and ignore the deleting key.
---
Toshiyuki Okajima
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 11:33 ` David Howells
2010-04-23 15:23 ` Toshiyuki Okajima
2010-04-23 15:52 ` David Howells
@ 2010-04-26 10:57 ` David Howells
2010-04-26 14:42 ` Toshiyuki Okajima
2 siblings, 1 reply; 13+ messages in thread
From: David Howells @ 2010-04-26 10:57 UTC (permalink / raw)
To: Toshiyuki Okajima
Cc: dhowells, keyrings, security, linux-security-module, linux-kernel
Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
> And my previous figure description(in first patch) was a bit wrong.
> Please replace it with my new one:
The spacing is all over the place, I'm afraid. Can you send it again?
For example, the vertical bars in the right-hand column don't align:
| [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()
| | |||
David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 15:52 ` David Howells
2010-04-24 0:32 ` 岡嶋 寿行
@ 2010-04-26 14:22 ` Toshiyuki Okajima
2010-04-26 14:47 ` David Howells
2 siblings, 0 replies; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-26 14:22 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, security, linux-security-module, linux-kernel
Hi, David.
# Sorry, I sent a mail which included local codes (Japanese) on Saturday.
# So, I resend this mail.
> Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
>
>> > Better still, atomic_inc_not_zero(). How about the attached patch?
>> Your fix looks good to me. But, if usage count of the keyring is 0,
>> I think it better to return -ENOKEY immediately.
>
> The problem with that is that someone else may have created a keyring with
> the
> same name that you can't then reach until the dead keyring is deleted.
OK. I understand. So, your patch looks fine to me.
---
Once find_keyring_by_name() returns -ENOKEY, the new user creates a
new key. So, both the deleting key and the new key may exist.
Therefore at next find_keyring_by_name() call, we should find the new
key and ignore the deleting key.
---
Today, I tested your patch and any problem didn't happen.
So, I rewrote your patch. Here you are:
---
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Subject: [PATCH] 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: [<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.
Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Signed-off-by: David Howells <dhowells@redhat.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() */
---
Best Regards,
Toshiyuki Okajima
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-26 10:57 ` David Howells
@ 2010-04-26 14:42 ` Toshiyuki Okajima
2010-04-29 11:23 ` Toshiyuki Okajima
0 siblings, 1 reply; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-26 14:42 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, security, linux-security-module, linux-kernel
Hi, David.
Sorry, I missed your last mail.
On Mon, 26 Apr 2010 11:57:36 +0100
David Howells <dhowells@redhat.com> wrote:
> Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
>
> > And my previous figure description(in first patch) was a bit wrong.
> > Please replace it with my new one:
>
> The spacing is all over the place, I'm afraid. Can you send it again?
>
> For example, the vertical bars in the right-hand column don't align:
>
> | [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()
> | | |||
>
So, I applied your commets and rewrite your patch.
Here you are:
---
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Subject: [PATCH] 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: [<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.
Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Signed-off-by: David Howells <dhowells@redhat.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() */
--
Best Regards,
Toshiyuki Okajima
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-23 15:52 ` David Howells
2010-04-24 0:32 ` 岡嶋 寿行
2010-04-26 14:22 ` Toshiyuki Okajima
@ 2010-04-26 14:47 ` David Howells
2 siblings, 0 replies; 13+ messages in thread
From: David Howells @ 2010-04-26 14:47 UTC (permalink / raw)
To: Toshiyuki Okajima
Cc: dhowells, keyrings, security, linux-security-module, linux-kernel
Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote:
> OK. I understand. So, your patch looks fine to me.
> ---
> Once find_keyring_by_name() returns -ENOKEY, the new user creates a
> new key. So, both the deleting key and the new key may exist.
> Therefore at next find_keyring_by_name() call, we should find the new
> key and ignore the deleting key.
> ---
>
> Today, I tested your patch and any problem didn't happen.
> So, I rewrote your patch. Here you are:
Thanks.
Note that the vertical lines in the right-hand column of your timeline don't
seem to line up correctly. Are you editing it in an editor with a
proportional font?
David
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1][BUG][TAKE2] KEYRINGS: find_keyring_by_name() can gain the freed keyring
2010-04-26 14:42 ` Toshiyuki Okajima
@ 2010-04-29 11:23 ` Toshiyuki Okajima
0 siblings, 0 replies; 13+ messages in thread
From: Toshiyuki Okajima @ 2010-04-29 11:23 UTC (permalink / raw)
To: David Howells
Cc: keyrings, security, linux-security-module, linux-kernel,
Toshiyuki Okajima
[-- Attachment #1: Type: text/plain, Size: 398 bytes --]
Hi, David.
> Note that the vertical lines in the right-hand column of your timeline don't
> seem to line up correctly. Are you editing it in an editor with a
> proportional font?
I thought that this problem (the right-hand column seemed not to be correct)
had been already fixed.
But I couldn't find out this reason. So, I attach the fixed patch on this mail.
--
Best Regards,
Toshiyuki Okajima
[-- Attachment #2: your_patch.txt --]
[-- Type: text/plain, Size: 8594 bytes --]
From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Subject: [PATCH] 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: [<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.
Tested-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Reported-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Signed-off-by: David Howells <dhowells@redhat.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() */
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-05-01 0:00 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-22 7:37 [PATCH 1/1][BUG][IMPORTANT] KEYRINGS: find_keyring_by_name() can gain the freed keyring Toshiyuki Okajima
2010-04-22 10:16 ` David Howells
2010-04-23 10:45 ` Toshiyuki Okajima
2010-04-23 10:51 ` [PATCH 1/1][BUG][TAKE2] " Toshiyuki Okajima
2010-04-23 11:33 ` David Howells
2010-04-23 15:23 ` Toshiyuki Okajima
2010-04-23 15:52 ` David Howells
2010-04-24 0:32 ` 岡嶋 寿行
2010-04-26 14:22 ` Toshiyuki Okajima
2010-04-26 14:47 ` David Howells
2010-04-26 10:57 ` David Howells
2010-04-26 14:42 ` Toshiyuki Okajima
2010-04-29 11:23 ` Toshiyuki Okajima
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.