Re: [PATCH] security/keys: fix slab-out-of-bounds in key_task_permission

From: chenridong
Date: Mon Oct 07 2024 - 21:41:10 EST




On 2024/10/8 7:15, Jarkko Sakkinen wrote:
Hi,

Revisit...

On Fri, 2024-09-13 at 07:09 +0000, Chen Ridong wrote:
We meet the same issue with the LINK, which reads memory out of
bounds:

Never ever use pronoun "we" in a commit message in any possible
sentence. Instead always use passive imperative.

What you probably want to say is:

"KASAN reports an out of bounds read:"

Right?


Yes.

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

Snip all below away:

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

Remember to cut only the relevant part of the stack trace to make this
commit message more compact and readable.

Thank you, I will do that.


However, we can't reproduce this issue.
After our analysis, it can make this issue by following steps.
1.As syzkaller reported, the memory is allocated for struct

"1."

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.

Delete the whole list and write a description of the problem and why
your change resolves it.

As per code change, let's layout it something more readable first:

/* Traverse branches into depth: */
if (assoc_array_ptr_is_meta(ptr)) {
if (node->back_pointer || assoc_array_ptr_is_shortcut(ptr))
goto descend_to_node;
}

So one thing that should be explained just to make the description
rigid is why 'ptr' is passed to assoc_array_ptr_is_shortcut() and
not 'node'. I'm actually 100% sure about that part, which kind
of supports my view here, right? :-)

The first part of the if-statement obviously filters out everything
that is not root (when it comes to 'node'). Explain the second part.
At that point it is know that node is a root node, so continue from
there.

BR, Jarkko


Thank you for your patience.
I will update soon.

Best regards,
Ridong