| Message ID | 20240913070928.1670785-1-chenridong@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | security/keys: fix slab-out-of-bounds in key_task_permission | expand |
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; }
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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))
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(-)