Re: [PATCH] security/keys: fix slab-out-of-bounds in key_task_permission
From: Chen Ridong
Date: Sat Sep 14 2024 - 20:55:25 EST
On 2024/9/14 19:33, Jarkko Sakkinen wrote:
On Fri Sep 13, 2024 at 10:09 AM EEST, Chen Ridong wrote:
We meet the same issue with the LINK, which reads memory out of bounds:
Nit: don't use "we" anywhere".
Tbh, I really don't understand the sentence above. I don't what
"the same issue with the LINK" really is.
Hello, Jarkko.
I apologize for any confusion caused.
I've encountered a bug reported by syzkaller. I also found the same bug
reported at this LINK:
https://syzkaller.appspot.com/bug?id=68a5e206c2a8e08d317eb83f05610c0484ad10b9.
BUG: KASAN: slab-out-of-bounds in __kuid_val include/linux/uidgid.h:36
BUG: KASAN: slab-out-of-bounds in uid_eq include/linux/uidgid.h:63 [inline]
BUG: KASAN: slab-out-of-bounds in key_task_permission+0x394/0x410
security/keys/permission.c:54
Read of size 4 at addr ffff88813c3ab618 by task stress-ng/4362
CPU: 2 PID: 4362 Comm: stress-ng Not tainted 5.10.0-14930-gafbffd6c3ede #15
Call Trace:
__dump_stack lib/dump_stack.c:82 [inline]
dump_stack+0x107/0x167 lib/dump_stack.c:123
print_address_description.constprop.0+0x19/0x170 mm/kasan/report.c:400
__kasan_report.cold+0x6c/0x84 mm/kasan/report.c:560
kasan_report+0x3a/0x50 mm/kasan/report.c:585
__kuid_val include/linux/uidgid.h:36 [inline]
uid_eq include/linux/uidgid.h:63 [inline]
key_task_permission+0x394/0x410 security/keys/permission.c:54
search_nested_keyrings+0x90e/0xe90 security/keys/keyring.c:793
keyring_search_rcu+0x1b6/0x310 security/keys/keyring.c:922
search_cred_keyrings_rcu+0x111/0x2e0 security/keys/process_keys.c:459
search_process_keyrings_rcu+0x1d/0x310 security/keys/process_keys.c:544
lookup_user_key+0x782/0x12e0 security/keys/process_keys.c:762
keyctl_invalidate_key+0x20/0x190 security/keys/keyctl.c:434
__do_sys_keyctl security/keys/keyctl.c:1978 [inline]
__se_sys_keyctl+0x1de/0x5b0 security/keys/keyctl.c:1880
do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x67/0xd1
However, we can't reproduce this issue.
"The issue cannot be easily reproduced but by analyzing the code
it can be broken into following steps:"
Thank you for your correction.
Does this patch address the issue correctly? Is this patch acceptable?
Best regard,
Ridong
After our analysis, it can make this issue by following steps.
1.As syzkaller reported, the memory is allocated for struct
assoc_array_shortcut in the assoc_array_insert_into_terminal_node
functions.
2.In the search_nested_keyrings, when we go through the slots in a node,
(bellow tag ascend_to_node), and the slot ptr is meta and
node->back_pointer != NULL, we will proceed to descend_to_node.
However, there is an exception. If node is the root, and one of the
slots points to a shortcut, it will be treated as a keyring.
3.Whether the ptr is keyring decided by keyring_ptr_is_keyring function.
However, KEYRING_PTR_SUBTYPE is 0x2UL, the same as
ASSOC_ARRAY_PTR_SUBTYPE_MASK,
4.As mentioned above, If a slot of the root is a shortcut, it may be
mistakenly be transferred to a key*, leading to an read out-of-bounds
read.
To fix this issue, one should jump to descend_to_node if the pointer is a
shortcut.
Link: https://syzkaller.appspot.com/bug?id=68a5e206c2a8e08d317eb83f05610c0484ad10b9
Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
---
security/keys/keyring.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4448758f643a..7958486ac834 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -772,7 +772,9 @@ static bool search_nested_keyrings(struct key *keyring,
for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
ptr = READ_ONCE(node->slots[slot]);
- if (assoc_array_ptr_is_meta(ptr) && node->back_pointer)
+ if ((assoc_array_ptr_is_meta(ptr) && node->back_pointer) ||
+ (assoc_array_ptr_is_meta(ptr) &&
+ assoc_array_ptr_is_shortcut(ptr)))
goto descend_to_node;
if (!keyring_ptr_is_keyring(ptr))
BR, Jarkko