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

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

Posted by Chen Ridong 2 months, 2 weeks ago

We meet the same issue with the LINK, which reads memory out of bounds:
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.
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@huawei.com>
---
 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))
-- 
2.34.1

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

Posted by Jarkko Sakkinen 1 month, 3 weeks ago

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?

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

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

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

Posted by chenridong 1 month, 3 weeks ago


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

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

Posted by Jarkko Sakkinen 1 month, 3 weeks ago

On Tue, 2024-10-08 at 09:40 +0800, chenridong wrote:
> 
> 
> 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.

Yeah of course, and I did low quality job earlier no issues admitting
that, so let's do this correct this time. I just try to describe
what I'm seeing as accurately as I can :-) 

Here it is just important to get the explanation and the code change
in-sync so that it is easy to verify and compare them, given that it
is quite sensitive functionality and somewhat obfuscated peace of code
showing age. 

Also I think a good is to make sure that every fix will leave it at
least a bit cleaner state. From this basis I proposed a bit different
layout for the code.

> 
> Best regards,
> Ridong

BR,Jarkko

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

Posted by Chen Ridong 1 month, 2 weeks ago


On 2024/10/8 10:41, Jarkko Sakkinen wrote:
> On Tue, 2024-10-08 at 09:40 +0800, chenridong wrote:
>>
>>
>> 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.
> 
> Yeah of course, and I did low quality job earlier no issues admitting
> that, so let's do this correct this time. I just try to describe
> what I'm seeing as accurately as I can :-)
> 
> Here it is just important to get the explanation and the code change
> in-sync so that it is easy to verify and compare them, given that it
> is quite sensitive functionality and somewhat obfuscated peace of code
> showing age.
> 
> Also I think a good is to make sure that every fix will leave it at
> least a bit cleaner state. From this basis I proposed a bit different
> layout for the code.
> 
>>
>> Best regards,
>> Ridong
> 
> BR,Jarkko

Hi, Jarkko, I sent the v2 several days ago.
I don't know whether you have received it. I hope you have time to look 
into it, and I am still looking forward to your reply.

v2: 
https://lore.kernel.org/linux-kernel/20241008124639.70000-1-chenridong@huaweicloud.com/

Best regards,
Ridong

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

Posted by Jarkko Sakkinen 2 months, 2 weeks ago

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.

> 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:"

> 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@huawei.com>
> ---
>  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

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

Posted by Chen Ridong 2 months, 2 weeks ago


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@huawei.com>
>> ---
>>   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

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

Posted by Jarkko Sakkinen 2 months, 2 weeks ago

On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
>
>
> 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?

I only comment new patch versions so not giving any promises but I can
say that it is I think definitely in the correct direction :-)

BR, Jarkko

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

Posted by Chen Ridong 2 months, 1 week ago


On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
>>
>>
>> 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?
> 
> I only comment new patch versions so not giving any promises but I can
> say that it is I think definitely in the correct direction :-)
> 
> BR, Jarkko

Hello, Jarkko. I have reproduced this issue. It can be reproduced by 
following these steps:

1. Add the helper patch.

@@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct 
keyring_index_key *index_key)
         else if (index_key->type == &key_type_keyring && (hash & 
fan_mask) != 0)
                 hash = (hash + (hash << level_shift)) & ~fan_mask;
         index_key->hash = hash;
+       if ((index_key->hash & 0xff) == 0xe6) {
+                       pr_err("hash_key_type_and_desc: type %s %s 
0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
+       }
  }

2. Pick up the inputs whose hash is xxe6 using the following cmd. If a 
key's hash is xxe6, it will be printed.

for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done

You have complile test_key whith following code.

#include <sys/types.h>
#include <keyutils.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int
main(int argc, char *argv[])
{
    key_serial_t key;

    if (argc != 4) {
	   fprintf(stderr, "Usage: %s type description payload\n",
			   argv[0]);
	   exit(EXIT_FAILURE);
    }

    key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
			   KEY_SPEC_SESSION_KEYRING);
    if (key == -1) {
	   perror("add_key");
	   exit(EXIT_FAILURE);
    }

    printf("Key ID is %jx\n", (uintmax_t) key);

    exit(EXIT_SUCCESS);
}


3. Have more than 32 inputs now. their hashes are xxe6.
eg.
hash_key_type_and_desc: type user user438 0xe3033fe6
hash_key_type_and_desc: type user user526 0xeb7eade6
...
hash_key_type_and_desc: type user user9955 0x44bc99e6

4. Reboot and add the keys obtained from step 3.
When adding keys to the ROOT that their hashes are all xxe6, and up to 
16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so 
the keys are dissimilar. The ROOT will then split NODE A without using a 
shortcut. When NODE A is filled with keys that have hashes of xxe6, the 
keys are similar. NODE A will split with a shortcut.

As my analysis, if a slot of the root is a shortcut(slot 6), it may be 
mistakenly be transferred to a key*, leading to an read out-of-bounds read.

                       NODE A
               +------>+---+
       ROOT    |       | 0 | xxe6
       +---+   |       +---+
  xxxx | 0 | shortcut  :   : xxe6
       +---+   |       +---+
  xxe6 :   :   |       |   | xxe6
       +---+   |       +---+
       | 6 |---+       :   : xxe6
       +---+           +---+
  xxe6 :   :           | f | xxe6
       +---+           +---+
  xxe6 | f |
       +---+

5. cat /proc/keys. and the issue is reproduced.

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

Posted by Jarkko Sakkinen 2 months, 1 week ago

On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
>
>
> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> > On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> >>
> >>
> >> 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?
> > 
> > I only comment new patch versions so not giving any promises but I can
> > say that it is I think definitely in the correct direction :-)
> > 
> > BR, Jarkko
>
> Hello, Jarkko. I have reproduced this issue. It can be reproduced by 
> following these steps:
>
> 1. Add the helper patch.
>
> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct 
> keyring_index_key *index_key)
>          else if (index_key->type == &key_type_keyring && (hash & 
> fan_mask) != 0)
>                  hash = (hash + (hash << level_shift)) & ~fan_mask;
>          index_key->hash = hash;
> +       if ((index_key->hash & 0xff) == 0xe6) {
> +                       pr_err("hash_key_type_and_desc: type %s %s 
> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
> +       }
>   }
>
> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a 
> key's hash is xxe6, it will be printed.
>
> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
>
> You have complile test_key whith following code.
>
> #include <sys/types.h>
> #include <keyutils.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int
> main(int argc, char *argv[])
> {
>     key_serial_t key;
>
>     if (argc != 4) {
> 	   fprintf(stderr, "Usage: %s type description payload\n",
> 			   argv[0]);
> 	   exit(EXIT_FAILURE);
>     }
>
>     key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
> 			   KEY_SPEC_SESSION_KEYRING);
>     if (key == -1) {
> 	   perror("add_key");
> 	   exit(EXIT_FAILURE);
>     }
>
>     printf("Key ID is %jx\n", (uintmax_t) key);
>
>     exit(EXIT_SUCCESS);
> }
>
>
> 3. Have more than 32 inputs now. their hashes are xxe6.
> eg.
> hash_key_type_and_desc: type user user438 0xe3033fe6
> hash_key_type_and_desc: type user user526 0xeb7eade6
> ...
> hash_key_type_and_desc: type user user9955 0x44bc99e6
>
> 4. Reboot and add the keys obtained from step 3.
> When adding keys to the ROOT that their hashes are all xxe6, and up to 
> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so 
> the keys are dissimilar. The ROOT will then split NODE A without using a 
> shortcut. When NODE A is filled with keys that have hashes of xxe6, the 
> keys are similar. NODE A will split with a shortcut.
>
> As my analysis, if a slot of the root is a shortcut(slot 6), it may be 
> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
>
>                        NODE A
>                +------>+---+
>        ROOT    |       | 0 | xxe6
>        +---+   |       +---+
>   xxxx | 0 | shortcut  :   : xxe6
>        +---+   |       +---+
>   xxe6 :   :   |       |   | xxe6
>        +---+   |       +---+
>        | 6 |---+       :   : xxe6
>        +---+           +---+
>   xxe6 :   :           | f | xxe6
>        +---+           +---+
>   xxe6 | f |
>        +---+
>
> 5. cat /proc/keys. and the issue is reproduced.

Hi, I'll try to run through your procedure next week and give my comments. 
Thanks for doing this.

BR, Jarkko

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

Posted by Chen Ridong 2 months ago

On 2024/9/19 4:57, Jarkko Sakkinen wrote:
> On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
>>
>>
>> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
>>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
>>>>
>>>>
>>>> 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?
>>>
>>> I only comment new patch versions so not giving any promises but I can
>>> say that it is I think definitely in the correct direction :-)
>>>
>>> BR, Jarkko
>>
>> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
>> following these steps:
>>
>> 1. Add the helper patch.
>>
>> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
>> keyring_index_key *index_key)
>>           else if (index_key->type == &key_type_keyring && (hash &
>> fan_mask) != 0)
>>                   hash = (hash + (hash << level_shift)) & ~fan_mask;
>>           index_key->hash = hash;
>> +       if ((index_key->hash & 0xff) == 0xe6) {
>> +                       pr_err("hash_key_type_and_desc: type %s %s
>> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
>> +       }
>>    }
>>
>> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
>> key's hash is xxe6, it will be printed.
>>
>> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
>>
>> You have complile test_key whith following code.
>>
>> #include <sys/types.h>
>> #include <keyutils.h>
>> #include <stdint.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>>
>> int
>> main(int argc, char *argv[])
>> {
>>      key_serial_t key;
>>
>>      if (argc != 4) {
>> 	   fprintf(stderr, "Usage: %s type description payload\n",
>> 			   argv[0]);
>> 	   exit(EXIT_FAILURE);
>>      }
>>
>>      key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
>> 			   KEY_SPEC_SESSION_KEYRING);
>>      if (key == -1) {
>> 	   perror("add_key");
>> 	   exit(EXIT_FAILURE);
>>      }
>>
>>      printf("Key ID is %jx\n", (uintmax_t) key);
>>
>>      exit(EXIT_SUCCESS);
>> }
>>
>>
>> 3. Have more than 32 inputs now. their hashes are xxe6.
>> eg.
>> hash_key_type_and_desc: type user user438 0xe3033fe6
>> hash_key_type_and_desc: type user user526 0xeb7eade6
>> ...
>> hash_key_type_and_desc: type user user9955 0x44bc99e6
>>
>> 4. Reboot and add the keys obtained from step 3.
>> When adding keys to the ROOT that their hashes are all xxe6, and up to
>> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
>> the keys are dissimilar. The ROOT will then split NODE A without using a
>> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
>> keys are similar. NODE A will split with a shortcut.
>>
>> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
>> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
>>
>>                         NODE A
>>                 +------>+---+
>>         ROOT    |       | 0 | xxe6
>>         +---+   |       +---+
>>    xxxx | 0 | shortcut  :   : xxe6
>>         +---+   |       +---+
>>    xxe6 :   :   |       |   | xxe6
>>         +---+   |       +---+
>>         | 6 |---+       :   : xxe6
>>         +---+           +---+
>>    xxe6 :   :           | f | xxe6
>>         +---+           +---+
>>    xxe6 | f |
>>         +---+
>>
>> 5. cat /proc/keys. and the issue is reproduced.
> 
> Hi, I'll try to run through your procedure next week and give my comments.
> Thanks for doing this.
> 
> BR, Jarkko

Hi, Jarkko, have you run these procedure?
I have tested this patch with LTP and a pressure test(stress-ng --key), 
and this patch have fixed this issue. Additionally, no new bugs have 
been found so far.

I am looking forward to your reply.

Best regards,
Ridong

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

Posted by Jarkko Sakkinen 2 months ago

On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
>
> On 2024/9/19 4:57, Jarkko Sakkinen wrote:
> > On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
> >>
> >>
> >> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> >>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> >>>>
> >>>>
> >>>> 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?
> >>>
> >>> I only comment new patch versions so not giving any promises but I can
> >>> say that it is I think definitely in the correct direction :-)
> >>>
> >>> BR, Jarkko
> >>
> >> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
> >> following these steps:
> >>
> >> 1. Add the helper patch.
> >>
> >> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
> >> keyring_index_key *index_key)
> >>           else if (index_key->type == &key_type_keyring && (hash &
> >> fan_mask) != 0)
> >>                   hash = (hash + (hash << level_shift)) & ~fan_mask;
> >>           index_key->hash = hash;
> >> +       if ((index_key->hash & 0xff) == 0xe6) {
> >> +                       pr_err("hash_key_type_and_desc: type %s %s
> >> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
> >> +       }
> >>    }
> >>
> >> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
> >> key's hash is xxe6, it will be printed.
> >>
> >> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
> >>
> >> You have complile test_key whith following code.
> >>
> >> #include <sys/types.h>
> >> #include <keyutils.h>
> >> #include <stdint.h>
> >> #include <stdio.h>
> >> #include <stdlib.h>
> >> #include <string.h>
> >>
> >> int
> >> main(int argc, char *argv[])
> >> {
> >>      key_serial_t key;
> >>
> >>      if (argc != 4) {
> >> 	   fprintf(stderr, "Usage: %s type description payload\n",
> >> 			   argv[0]);
> >> 	   exit(EXIT_FAILURE);
> >>      }
> >>
> >>      key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
> >> 			   KEY_SPEC_SESSION_KEYRING);
> >>      if (key == -1) {
> >> 	   perror("add_key");
> >> 	   exit(EXIT_FAILURE);
> >>      }
> >>
> >>      printf("Key ID is %jx\n", (uintmax_t) key);
> >>
> >>      exit(EXIT_SUCCESS);
> >> }
> >>
> >>
> >> 3. Have more than 32 inputs now. their hashes are xxe6.
> >> eg.
> >> hash_key_type_and_desc: type user user438 0xe3033fe6
> >> hash_key_type_and_desc: type user user526 0xeb7eade6
> >> ...
> >> hash_key_type_and_desc: type user user9955 0x44bc99e6
> >>
> >> 4. Reboot and add the keys obtained from step 3.
> >> When adding keys to the ROOT that their hashes are all xxe6, and up to
> >> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
> >> the keys are dissimilar. The ROOT will then split NODE A without using a
> >> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
> >> keys are similar. NODE A will split with a shortcut.
> >>
> >> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
> >> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
> >>
> >>                         NODE A
> >>                 +------>+---+
> >>         ROOT    |       | 0 | xxe6
> >>         +---+   |       +---+
> >>    xxxx | 0 | shortcut  :   : xxe6
> >>         +---+   |       +---+
> >>    xxe6 :   :   |       |   | xxe6
> >>         +---+   |       +---+
> >>         | 6 |---+       :   : xxe6
> >>         +---+           +---+
> >>    xxe6 :   :           | f | xxe6
> >>         +---+           +---+
> >>    xxe6 | f |
> >>         +---+
> >>
> >> 5. cat /proc/keys. and the issue is reproduced.
> > 
> > Hi, I'll try to run through your procedure next week and give my comments.
> > Thanks for doing this.
> > 
> > BR, Jarkko
>
> Hi, Jarkko, have you run these procedure?
> I have tested this patch with LTP and a pressure test(stress-ng --key), 
> and this patch have fixed this issue. Additionally, no new bugs have 
> been found so far.
>
> I am looking forward to your reply.
>
> Best regards,
> Ridong

Nope because we are apparently stuck with release critical bug:

https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t

Might take several weeks before I look into this.


BR, Jarkko

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

Posted by Jarkko Sakkinen 2 months ago

On Thu Sep 26, 2024 at 11:53 AM EEST, Jarkko Sakkinen wrote:
> On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
> >
> > On 2024/9/19 4:57, Jarkko Sakkinen wrote:
> > > On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
> > >>
> > >>
> > >> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> > >>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> > >>>>
> > >>>>
> > >>>> 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?
> > >>>
> > >>> I only comment new patch versions so not giving any promises but I can
> > >>> say that it is I think definitely in the correct direction :-)
> > >>>
> > >>> BR, Jarkko
> > >>
> > >> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
> > >> following these steps:
> > >>
> > >> 1. Add the helper patch.
> > >>
> > >> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
> > >> keyring_index_key *index_key)
> > >>           else if (index_key->type == &key_type_keyring && (hash &
> > >> fan_mask) != 0)
> > >>                   hash = (hash + (hash << level_shift)) & ~fan_mask;
> > >>           index_key->hash = hash;
> > >> +       if ((index_key->hash & 0xff) == 0xe6) {
> > >> +                       pr_err("hash_key_type_and_desc: type %s %s
> > >> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
> > >> +       }
> > >>    }
> > >>
> > >> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
> > >> key's hash is xxe6, it will be printed.
> > >>
> > >> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
> > >>
> > >> You have complile test_key whith following code.
> > >>
> > >> #include <sys/types.h>
> > >> #include <keyutils.h>
> > >> #include <stdint.h>
> > >> #include <stdio.h>
> > >> #include <stdlib.h>
> > >> #include <string.h>
> > >>
> > >> int
> > >> main(int argc, char *argv[])
> > >> {
> > >>      key_serial_t key;
> > >>
> > >>      if (argc != 4) {
> > >> 	   fprintf(stderr, "Usage: %s type description payload\n",
> > >> 			   argv[0]);
> > >> 	   exit(EXIT_FAILURE);
> > >>      }
> > >>
> > >>      key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
> > >> 			   KEY_SPEC_SESSION_KEYRING);
> > >>      if (key == -1) {
> > >> 	   perror("add_key");
> > >> 	   exit(EXIT_FAILURE);
> > >>      }
> > >>
> > >>      printf("Key ID is %jx\n", (uintmax_t) key);
> > >>
> > >>      exit(EXIT_SUCCESS);
> > >> }
> > >>
> > >>
> > >> 3. Have more than 32 inputs now. their hashes are xxe6.
> > >> eg.
> > >> hash_key_type_and_desc: type user user438 0xe3033fe6
> > >> hash_key_type_and_desc: type user user526 0xeb7eade6
> > >> ...
> > >> hash_key_type_and_desc: type user user9955 0x44bc99e6
> > >>
> > >> 4. Reboot and add the keys obtained from step 3.
> > >> When adding keys to the ROOT that their hashes are all xxe6, and up to
> > >> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
> > >> the keys are dissimilar. The ROOT will then split NODE A without using a
> > >> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
> > >> keys are similar. NODE A will split with a shortcut.
> > >>
> > >> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
> > >> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
> > >>
> > >>                         NODE A
> > >>                 +------>+---+
> > >>         ROOT    |       | 0 | xxe6
> > >>         +---+   |       +---+
> > >>    xxxx | 0 | shortcut  :   : xxe6
> > >>         +---+   |       +---+
> > >>    xxe6 :   :   |       |   | xxe6
> > >>         +---+   |       +---+
> > >>         | 6 |---+       :   : xxe6
> > >>         +---+           +---+
> > >>    xxe6 :   :           | f | xxe6
> > >>         +---+           +---+
> > >>    xxe6 | f |
> > >>         +---+
> > >>
> > >> 5. cat /proc/keys. and the issue is reproduced.
> > > 
> > > Hi, I'll try to run through your procedure next week and give my comments.
> > > Thanks for doing this.
> > > 
> > > BR, Jarkko
> >
> > Hi, Jarkko, have you run these procedure?
> > I have tested this patch with LTP and a pressure test(stress-ng --key), 
> > and this patch have fixed this issue. Additionally, no new bugs have 
> > been found so far.
> >
> > I am looking forward to your reply.
> >
> > Best regards,
> > Ridong
>
> Nope because we are apparently stuck with release critical bug:
>
> https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t
>
> Might take several weeks before I look into this.

I was expecting to send a PR early this week since the patch set
addresses the issue so thus wrong estimation.

BR, Jarkko

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

Posted by Jarkko Sakkinen 2 months ago

On Thu Sep 26, 2024 at 11:55 AM EEST, Jarkko Sakkinen wrote:
> On Thu Sep 26, 2024 at 11:53 AM EEST, Jarkko Sakkinen wrote:
> > On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
> > >
> > > On 2024/9/19 4:57, Jarkko Sakkinen wrote:
> > > > On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
> > > >>
> > > >>
> > > >> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> > > >>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> > > >>>>
> > > >>>>
> > > >>>> 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?
> > > >>>
> > > >>> I only comment new patch versions so not giving any promises but I can
> > > >>> say that it is I think definitely in the correct direction :-)
> > > >>>
> > > >>> BR, Jarkko
> > > >>
> > > >> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
> > > >> following these steps:
> > > >>
> > > >> 1. Add the helper patch.
> > > >>
> > > >> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
> > > >> keyring_index_key *index_key)
> > > >>           else if (index_key->type == &key_type_keyring && (hash &
> > > >> fan_mask) != 0)
> > > >>                   hash = (hash + (hash << level_shift)) & ~fan_mask;
> > > >>           index_key->hash = hash;
> > > >> +       if ((index_key->hash & 0xff) == 0xe6) {
> > > >> +                       pr_err("hash_key_type_and_desc: type %s %s
> > > >> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
> > > >> +       }
> > > >>    }
> > > >>
> > > >> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
> > > >> key's hash is xxe6, it will be printed.
> > > >>
> > > >> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
> > > >>
> > > >> You have complile test_key whith following code.
> > > >>
> > > >> #include <sys/types.h>
> > > >> #include <keyutils.h>
> > > >> #include <stdint.h>
> > > >> #include <stdio.h>
> > > >> #include <stdlib.h>
> > > >> #include <string.h>
> > > >>
> > > >> int
> > > >> main(int argc, char *argv[])
> > > >> {
> > > >>      key_serial_t key;
> > > >>
> > > >>      if (argc != 4) {
> > > >> 	   fprintf(stderr, "Usage: %s type description payload\n",
> > > >> 			   argv[0]);
> > > >> 	   exit(EXIT_FAILURE);
> > > >>      }
> > > >>
> > > >>      key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
> > > >> 			   KEY_SPEC_SESSION_KEYRING);
> > > >>      if (key == -1) {
> > > >> 	   perror("add_key");
> > > >> 	   exit(EXIT_FAILURE);
> > > >>      }
> > > >>
> > > >>      printf("Key ID is %jx\n", (uintmax_t) key);
> > > >>
> > > >>      exit(EXIT_SUCCESS);
> > > >> }
> > > >>
> > > >>
> > > >> 3. Have more than 32 inputs now. their hashes are xxe6.
> > > >> eg.
> > > >> hash_key_type_and_desc: type user user438 0xe3033fe6
> > > >> hash_key_type_and_desc: type user user526 0xeb7eade6
> > > >> ...
> > > >> hash_key_type_and_desc: type user user9955 0x44bc99e6
> > > >>
> > > >> 4. Reboot and add the keys obtained from step 3.
> > > >> When adding keys to the ROOT that their hashes are all xxe6, and up to
> > > >> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
> > > >> the keys are dissimilar. The ROOT will then split NODE A without using a
> > > >> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
> > > >> keys are similar. NODE A will split with a shortcut.
> > > >>
> > > >> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
> > > >> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
> > > >>
> > > >>                         NODE A
> > > >>                 +------>+---+
> > > >>         ROOT    |       | 0 | xxe6
> > > >>         +---+   |       +---+
> > > >>    xxxx | 0 | shortcut  :   : xxe6
> > > >>         +---+   |       +---+
> > > >>    xxe6 :   :   |       |   | xxe6
> > > >>         +---+   |       +---+
> > > >>         | 6 |---+       :   : xxe6
> > > >>         +---+           +---+
> > > >>    xxe6 :   :           | f | xxe6
> > > >>         +---+           +---+
> > > >>    xxe6 | f |
> > > >>         +---+
> > > >>
> > > >> 5. cat /proc/keys. and the issue is reproduced.
> > > > 
> > > > Hi, I'll try to run through your procedure next week and give my comments.
> > > > Thanks for doing this.
> > > > 
> > > > BR, Jarkko
> > >
> > > Hi, Jarkko, have you run these procedure?
> > > I have tested this patch with LTP and a pressure test(stress-ng --key), 
> > > and this patch have fixed this issue. Additionally, no new bugs have 
> > > been found so far.
> > >
> > > I am looking forward to your reply.
> > >
> > > Best regards,
> > > Ridong
> >
> > Nope because we are apparently stuck with release critical bug:
> >
> > https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t
> >
> > Might take several weeks before I look into this.
>
> I was expecting to send a PR early this week since the patch set
> addresses the issue so thus wrong estimation.

I asked David if he could look into this.

BR, Jarkko

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

Posted by chenridong 2 months ago


On 2024/9/26 17:54, Jarkko Sakkinen wrote:
> On Thu Sep 26, 2024 at 11:55 AM EEST, Jarkko Sakkinen wrote:
>> On Thu Sep 26, 2024 at 11:53 AM EEST, Jarkko Sakkinen wrote:
>>> On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
>>>>
>>>> On 2024/9/19 4:57, Jarkko Sakkinen wrote:
>>>>> On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
>>>>>>
>>>>>>
>>>>>> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
>>>>>>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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?
>>>>>>>
>>>>>>> I only comment new patch versions so not giving any promises but I can
>>>>>>> say that it is I think definitely in the correct direction :-)
>>>>>>>
>>>>>>> BR, Jarkko
>>>>>>
>>>>>> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
>>>>>> following these steps:
>>>>>>
>>>>>> 1. Add the helper patch.
>>>>>>
>>>>>> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
>>>>>> keyring_index_key *index_key)
>>>>>>            else if (index_key->type == &key_type_keyring && (hash &
>>>>>> fan_mask) != 0)
>>>>>>                    hash = (hash + (hash << level_shift)) & ~fan_mask;
>>>>>>            index_key->hash = hash;
>>>>>> +       if ((index_key->hash & 0xff) == 0xe6) {
>>>>>> +                       pr_err("hash_key_type_and_desc: type %s %s
>>>>>> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
>>>>>> +       }
>>>>>>     }
>>>>>>
>>>>>> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
>>>>>> key's hash is xxe6, it will be printed.
>>>>>>
>>>>>> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
>>>>>>
>>>>>> You have complile test_key whith following code.
>>>>>>
>>>>>> #include <sys/types.h>
>>>>>> #include <keyutils.h>
>>>>>> #include <stdint.h>
>>>>>> #include <stdio.h>
>>>>>> #include <stdlib.h>
>>>>>> #include <string.h>
>>>>>>
>>>>>> int
>>>>>> main(int argc, char *argv[])
>>>>>> {
>>>>>>       key_serial_t key;
>>>>>>
>>>>>>       if (argc != 4) {
>>>>>> 	   fprintf(stderr, "Usage: %s type description payload\n",
>>>>>> 			   argv[0]);
>>>>>> 	   exit(EXIT_FAILURE);
>>>>>>       }
>>>>>>
>>>>>>       key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
>>>>>> 			   KEY_SPEC_SESSION_KEYRING);
>>>>>>       if (key == -1) {
>>>>>> 	   perror("add_key");
>>>>>> 	   exit(EXIT_FAILURE);
>>>>>>       }
>>>>>>
>>>>>>       printf("Key ID is %jx\n", (uintmax_t) key);
>>>>>>
>>>>>>       exit(EXIT_SUCCESS);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> 3. Have more than 32 inputs now. their hashes are xxe6.
>>>>>> eg.
>>>>>> hash_key_type_and_desc: type user user438 0xe3033fe6
>>>>>> hash_key_type_and_desc: type user user526 0xeb7eade6
>>>>>> ...
>>>>>> hash_key_type_and_desc: type user user9955 0x44bc99e6
>>>>>>
>>>>>> 4. Reboot and add the keys obtained from step 3.
>>>>>> When adding keys to the ROOT that their hashes are all xxe6, and up to
>>>>>> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
>>>>>> the keys are dissimilar. The ROOT will then split NODE A without using a
>>>>>> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
>>>>>> keys are similar. NODE A will split with a shortcut.
>>>>>>
>>>>>> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
>>>>>> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
>>>>>>
>>>>>>                          NODE A
>>>>>>                  +------>+---+
>>>>>>          ROOT    |       | 0 | xxe6
>>>>>>          +---+   |       +---+
>>>>>>     xxxx | 0 | shortcut  :   : xxe6
>>>>>>          +---+   |       +---+
>>>>>>     xxe6 :   :   |       |   | xxe6
>>>>>>          +---+   |       +---+
>>>>>>          | 6 |---+       :   : xxe6
>>>>>>          +---+           +---+
>>>>>>     xxe6 :   :           | f | xxe6
>>>>>>          +---+           +---+
>>>>>>     xxe6 | f |
>>>>>>          +---+
>>>>>>
>>>>>> 5. cat /proc/keys. and the issue is reproduced.
>>>>>
>>>>> Hi, I'll try to run through your procedure next week and give my comments.
>>>>> Thanks for doing this.
>>>>>
>>>>> BR, Jarkko
>>>>
>>>> Hi, Jarkko, have you run these procedure?
>>>> I have tested this patch with LTP and a pressure test(stress-ng --key),
>>>> and this patch have fixed this issue. Additionally, no new bugs have
>>>> been found so far.
>>>>
>>>> I am looking forward to your reply.
>>>>
>>>> Best regards,
>>>> Ridong
>>>
>>> Nope because we are apparently stuck with release critical bug:
>>>
>>> https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t
>>>
>>> Might take several weeks before I look into this.
>>
>> I was expecting to send a PR early this week since the patch set
>> addresses the issue so thus wrong estimation.
> 
> I asked David if he could look into this.
> 
> BR, Jarkko

Thank you very much.

Best regards,
Ridong

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

Posted by Jarkko Sakkinen 2 months ago

On Thu Sep 26, 2024 at 2:20 PM EEST, chenridong wrote:
>
>
> On 2024/9/26 17:54, Jarkko Sakkinen wrote:
> > On Thu Sep 26, 2024 at 11:55 AM EEST, Jarkko Sakkinen wrote:
> >> On Thu Sep 26, 2024 at 11:53 AM EEST, Jarkko Sakkinen wrote:
> >>> On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
> >>>>
> >>>> On 2024/9/19 4:57, Jarkko Sakkinen wrote:
> >>>>> On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
> >>>>>>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 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?
> >>>>>>>
> >>>>>>> I only comment new patch versions so not giving any promises but I can
> >>>>>>> say that it is I think definitely in the correct direction :-)
> >>>>>>>
> >>>>>>> BR, Jarkko
> >>>>>>
> >>>>>> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
> >>>>>> following these steps:
> >>>>>>
> >>>>>> 1. Add the helper patch.
> >>>>>>
> >>>>>> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
> >>>>>> keyring_index_key *index_key)
> >>>>>>            else if (index_key->type == &key_type_keyring && (hash &
> >>>>>> fan_mask) != 0)
> >>>>>>                    hash = (hash + (hash << level_shift)) & ~fan_mask;
> >>>>>>            index_key->hash = hash;
> >>>>>> +       if ((index_key->hash & 0xff) == 0xe6) {
> >>>>>> +                       pr_err("hash_key_type_and_desc: type %s %s
> >>>>>> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
> >>>>>> +       }
> >>>>>>     }
> >>>>>>
> >>>>>> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
> >>>>>> key's hash is xxe6, it will be printed.
> >>>>>>
> >>>>>> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
> >>>>>>
> >>>>>> You have complile test_key whith following code.
> >>>>>>
> >>>>>> #include <sys/types.h>
> >>>>>> #include <keyutils.h>
> >>>>>> #include <stdint.h>
> >>>>>> #include <stdio.h>
> >>>>>> #include <stdlib.h>
> >>>>>> #include <string.h>
> >>>>>>
> >>>>>> int
> >>>>>> main(int argc, char *argv[])
> >>>>>> {
> >>>>>>       key_serial_t key;
> >>>>>>
> >>>>>>       if (argc != 4) {
> >>>>>> 	   fprintf(stderr, "Usage: %s type description payload\n",
> >>>>>> 			   argv[0]);
> >>>>>> 	   exit(EXIT_FAILURE);
> >>>>>>       }
> >>>>>>
> >>>>>>       key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
> >>>>>> 			   KEY_SPEC_SESSION_KEYRING);
> >>>>>>       if (key == -1) {
> >>>>>> 	   perror("add_key");
> >>>>>> 	   exit(EXIT_FAILURE);
> >>>>>>       }
> >>>>>>
> >>>>>>       printf("Key ID is %jx\n", (uintmax_t) key);
> >>>>>>
> >>>>>>       exit(EXIT_SUCCESS);
> >>>>>> }
> >>>>>>
> >>>>>>
> >>>>>> 3. Have more than 32 inputs now. their hashes are xxe6.
> >>>>>> eg.
> >>>>>> hash_key_type_and_desc: type user user438 0xe3033fe6
> >>>>>> hash_key_type_and_desc: type user user526 0xeb7eade6
> >>>>>> ...
> >>>>>> hash_key_type_and_desc: type user user9955 0x44bc99e6
> >>>>>>
> >>>>>> 4. Reboot and add the keys obtained from step 3.
> >>>>>> When adding keys to the ROOT that their hashes are all xxe6, and up to
> >>>>>> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
> >>>>>> the keys are dissimilar. The ROOT will then split NODE A without using a
> >>>>>> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
> >>>>>> keys are similar. NODE A will split with a shortcut.
> >>>>>>
> >>>>>> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
> >>>>>> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
> >>>>>>
> >>>>>>                          NODE A
> >>>>>>                  +------>+---+
> >>>>>>          ROOT    |       | 0 | xxe6
> >>>>>>          +---+   |       +---+
> >>>>>>     xxxx | 0 | shortcut  :   : xxe6
> >>>>>>          +---+   |       +---+
> >>>>>>     xxe6 :   :   |       |   | xxe6
> >>>>>>          +---+   |       +---+
> >>>>>>          | 6 |---+       :   : xxe6
> >>>>>>          +---+           +---+
> >>>>>>     xxe6 :   :           | f | xxe6
> >>>>>>          +---+           +---+
> >>>>>>     xxe6 | f |
> >>>>>>          +---+
> >>>>>>
> >>>>>> 5. cat /proc/keys. and the issue is reproduced.
> >>>>>
> >>>>> Hi, I'll try to run through your procedure next week and give my comments.
> >>>>> Thanks for doing this.
> >>>>>
> >>>>> BR, Jarkko
> >>>>
> >>>> Hi, Jarkko, have you run these procedure?
> >>>> I have tested this patch with LTP and a pressure test(stress-ng --key),
> >>>> and this patch have fixed this issue. Additionally, no new bugs have
> >>>> been found so far.
> >>>>
> >>>> I am looking forward to your reply.
> >>>>
> >>>> Best regards,
> >>>> Ridong
> >>>
> >>> Nope because we are apparently stuck with release critical bug:
> >>>
> >>> https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t
> >>>
> >>> Might take several weeks before I look into this.
> >>
> >> I was expecting to send a PR early this week since the patch set
> >> addresses the issue so thus wrong estimation.
> > 
> > I asked David if he could look into this.
> > 
> > BR, Jarkko
>
> Thank you very much.

Further, I'm switching jobs. Tomorrow is my last day in the current
job and next week starting a new job so given all these circumastances
I rather look into this properly hopefully latest after my rc2 PR is
out, rather than rushing.

In a normal status quo situation this would not be such a huge issue.

Similarly, for the performance bug I want to review James' comments
etc with time and bake v6 that hopefully satisfies all the
stateholders.

So thank you for understanding, and I appreciate the work you've done
on this. I.e. not ignoring this :-)

BR, Jarkko

BR, Jarkko

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

Posted by chenridong 2 months ago


On 2024/9/27 1:08, Jarkko Sakkinen wrote:
> On Thu Sep 26, 2024 at 2:20 PM EEST, chenridong wrote:
>>
>>
>> On 2024/9/26 17:54, Jarkko Sakkinen wrote:
>>> On Thu Sep 26, 2024 at 11:55 AM EEST, Jarkko Sakkinen wrote:
>>>> On Thu Sep 26, 2024 at 11:53 AM EEST, Jarkko Sakkinen wrote:
>>>>> On Thu Sep 26, 2024 at 6:48 AM EEST, Chen Ridong wrote:
>>>>>>
>>>>>> On 2024/9/19 4:57, Jarkko Sakkinen wrote:
>>>>>>> On Wed Sep 18, 2024 at 10:30 AM EEST, Chen Ridong wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2024/9/15 21:59, Jarkko Sakkinen wrote:
>>>>>>>>> On Sun Sep 15, 2024 at 3:55 AM EEST, Chen Ridong wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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?
>>>>>>>>>
>>>>>>>>> I only comment new patch versions so not giving any promises but I can
>>>>>>>>> say that it is I think definitely in the correct direction :-)
>>>>>>>>>
>>>>>>>>> BR, Jarkko
>>>>>>>>
>>>>>>>> Hello, Jarkko. I have reproduced this issue. It can be reproduced by
>>>>>>>> following these steps:
>>>>>>>>
>>>>>>>> 1. Add the helper patch.
>>>>>>>>
>>>>>>>> @@ -205,6 +205,9 @@ static void hash_key_type_and_desc(struct
>>>>>>>> keyring_index_key *index_key)
>>>>>>>>             else if (index_key->type == &key_type_keyring && (hash &
>>>>>>>> fan_mask) != 0)
>>>>>>>>                     hash = (hash + (hash << level_shift)) & ~fan_mask;
>>>>>>>>             index_key->hash = hash;
>>>>>>>> +       if ((index_key->hash & 0xff) == 0xe6) {
>>>>>>>> +                       pr_err("hash_key_type_and_desc: type %s %s
>>>>>>>> 0x%x\n",  index_key->type->name, index_key->description, index_key->hash);
>>>>>>>> +       }
>>>>>>>>      }
>>>>>>>>
>>>>>>>> 2. Pick up the inputs whose hash is xxe6 using the following cmd. If a
>>>>>>>> key's hash is xxe6, it will be printed.
>>>>>>>>
>>>>>>>> for ((i=0; i<=10000; i++)); do ./test_key user user$i "Some payload"; done
>>>>>>>>
>>>>>>>> You have complile test_key whith following code.
>>>>>>>>
>>>>>>>> #include <sys/types.h>
>>>>>>>> #include <keyutils.h>
>>>>>>>> #include <stdint.h>
>>>>>>>> #include <stdio.h>
>>>>>>>> #include <stdlib.h>
>>>>>>>> #include <string.h>
>>>>>>>>
>>>>>>>> int
>>>>>>>> main(int argc, char *argv[])
>>>>>>>> {
>>>>>>>>        key_serial_t key;
>>>>>>>>
>>>>>>>>        if (argc != 4) {
>>>>>>>> 	   fprintf(stderr, "Usage: %s type description payload\n",
>>>>>>>> 			   argv[0]);
>>>>>>>> 	   exit(EXIT_FAILURE);
>>>>>>>>        }
>>>>>>>>
>>>>>>>>        key = add_key(argv[1], argv[2], argv[3], strlen(argv[3]),
>>>>>>>> 			   KEY_SPEC_SESSION_KEYRING);
>>>>>>>>        if (key == -1) {
>>>>>>>> 	   perror("add_key");
>>>>>>>> 	   exit(EXIT_FAILURE);
>>>>>>>>        }
>>>>>>>>
>>>>>>>>        printf("Key ID is %jx\n", (uintmax_t) key);
>>>>>>>>
>>>>>>>>        exit(EXIT_SUCCESS);
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> 3. Have more than 32 inputs now. their hashes are xxe6.
>>>>>>>> eg.
>>>>>>>> hash_key_type_and_desc: type user user438 0xe3033fe6
>>>>>>>> hash_key_type_and_desc: type user user526 0xeb7eade6
>>>>>>>> ...
>>>>>>>> hash_key_type_and_desc: type user user9955 0x44bc99e6
>>>>>>>>
>>>>>>>> 4. Reboot and add the keys obtained from step 3.
>>>>>>>> When adding keys to the ROOT that their hashes are all xxe6, and up to
>>>>>>>> 16, the ROOT has keys with hashes that are not xxe6 (e.g., slot 0), so
>>>>>>>> the keys are dissimilar. The ROOT will then split NODE A without using a
>>>>>>>> shortcut. When NODE A is filled with keys that have hashes of xxe6, the
>>>>>>>> keys are similar. NODE A will split with a shortcut.
>>>>>>>>
>>>>>>>> As my analysis, if a slot of the root is a shortcut(slot 6), it may be
>>>>>>>> mistakenly be transferred to a key*, leading to an read out-of-bounds read.
>>>>>>>>
>>>>>>>>                           NODE A
>>>>>>>>                   +------>+---+
>>>>>>>>           ROOT    |       | 0 | xxe6
>>>>>>>>           +---+   |       +---+
>>>>>>>>      xxxx | 0 | shortcut  :   : xxe6
>>>>>>>>           +---+   |       +---+
>>>>>>>>      xxe6 :   :   |       |   | xxe6
>>>>>>>>           +---+   |       +---+
>>>>>>>>           | 6 |---+       :   : xxe6
>>>>>>>>           +---+           +---+
>>>>>>>>      xxe6 :   :           | f | xxe6
>>>>>>>>           +---+           +---+
>>>>>>>>      xxe6 | f |
>>>>>>>>           +---+
>>>>>>>>
>>>>>>>> 5. cat /proc/keys. and the issue is reproduced.
>>>>>>>
>>>>>>> Hi, I'll try to run through your procedure next week and give my comments.
>>>>>>> Thanks for doing this.
>>>>>>>
>>>>>>> BR, Jarkko
>>>>>>
>>>>>> Hi, Jarkko, have you run these procedure?
>>>>>> I have tested this patch with LTP and a pressure test(stress-ng --key),
>>>>>> and this patch have fixed this issue. Additionally, no new bugs have
>>>>>> been found so far.
>>>>>>
>>>>>> I am looking forward to your reply.
>>>>>>
>>>>>> Best regards,
>>>>>> Ridong
>>>>>
>>>>> Nope because we are apparently stuck with release critical bug:
>>>>>
>>>>> https://lore.kernel.org/linux-integrity/D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org/T/#t
>>>>>
>>>>> Might take several weeks before I look into this.
>>>>
>>>> I was expecting to send a PR early this week since the patch set
>>>> addresses the issue so thus wrong estimation.
>>>
>>> I asked David if he could look into this.
>>>
>>> BR, Jarkko
>>
>> Thank you very much.
> 
> Further, I'm switching jobs. Tomorrow is my last day in the current
> job and next week starting a new job so given all these circumastances
> I rather look into this properly hopefully latest after my rc2 PR is
> out, rather than rushing.
> 
> In a normal status quo situation this would not be such a huge issue.
> 
> Similarly, for the performance bug I want to review James' comments
> etc with time and bake v6 that hopefully satisfies all the
> stateholders.
> 
> So thank you for understanding, and I appreciate the work you've done
> on this. I.e. not ignoring this :-)
> 
> BR, Jarkko
> 
> BR, Jarkko

I am happy to fix this issue. I am also looking forward to your reply.
If this patch is acceptable, I will send a patch to update the commit 
message you have mentioned.

Best regards,
Ridong

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

Posted by Chen Ridong 2 months, 2 weeks ago


On 2024/9/13 15:09, Chen Ridong wrote:
> We meet the same issue with the LINK, which reads memory out of bounds:
> 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.
> 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@huawei.com>
> ---
>   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))

Should assoc_array_ptr_is_shortcut add ASSOC_ARRAY_PTR_TYPE_MASK 
judgement? Just like:

static inline bool assoc_array_ptr_is_shortcut(const struct 
assoc_array_ptr *x)
{
	return (unsigned long)x & ASSOC_ARRAY_PTR_TYPE_MASK &&
		   (unsigned long)x & ASSOC_ARRAY_PTR_SUBTYPE_MASK;
}