| Message ID | 20240913070928.1670785-1-chenridong@huawei.com (mailing list archive) |
|---|---|
| State | New |
| 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
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(-)