On 2024/9/14 19:33, Jarkko Sakkinen wrote:

On Fri Sep 13, 2024 at 10:09 AM EEST, Chen Ridong wrote:

quoted

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.

quoted

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

quoted

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