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

[Chen Ridong <chenridong@huaweicloud.com>]

On 2024/10/8 20:46, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> KASAN reports an out of bounds read:
> 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
> 
> This issue was also reported by syzbot [1].
> 
> It can be reproduced by following these steps(more details [2]):
> 1. Obtain more than 32 inputs that have similar hashes, which ends with the
>     pattern '0xxxxxxxe6'.
> 2. Reboot and add the keys obtained in step 1.
> 
> The reproducer demonstrates how this issue happened:
> 1. In the search_nested_keyrings function, when it iterates through the
>     slots in a node(below tag ascend_to_node), if the slot pointer is meta
>     and node->back_pointer != NULL(it means a root), it 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.
> 2. 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.
> 3. When 32 keys with the similar hashes are added to the tree, the ROOT
>     has keys with hashes that are not similar (e.g. slot 0) and it splits
>     NODE A without using a shortcut. When NODE A is filled with keys that
>     all hashes are xxe6, the keys are similar, NODE A will split with a
>     shortcut. Finally, it forms the tree as shown below, where slot 6 points
>     to a shortcut.
> 
>                        NODE A
>                +------>+---+
>        ROOT    |       | 0 | xxe6
>        +---+   |       +---+
>   xxxx | 0 | shortcut  :   : xxe6
>        +---+   |       +---+
>   xxe6 :   :   |       |   | xxe6
>        +---+   |       +---+
>        | 6 |---+       :   : xxe6
>        +---+           +---+
>   xxe6 :   :           | f | xxe6
>        +---+           +---+
>   xxe6 | f |
>        +---+
> 
> 4. As mentioned above, If a slot(slot 6) of the root points to a shortcut,
>     it may be mistakenly transferred to a key*, leading to a read
>     out-of-bounds read.
> 
> To fix this issue, one should jump to descend_to_node if the ptr is a
> shortcut, regardless of whether the node is root or not.
> 
> [1] https://lore.kernel.org/all/000000000000cbb7860611f61147@google.com/T/
> [2] https://lore.kernel.org/linux-kernel/1cfa878e-8c7b-4570-8606-21daf5e13ce7@huaweicloud.com/
> 
> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> Reported-by: syzbot+5b415c07907a2990d1a3@syzkaller.appspotmail.com
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
>   security/keys/keyring.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 4448758f643a..f331725d5a37 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -772,8 +772,11 @@ 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)
> -			goto descend_to_node;
> +		if (assoc_array_ptr_is_meta(ptr)) {
> +			if (node->back_pointer ||
> +			    assoc_array_ptr_is_shortcut(ptr))
> +				goto descend_to_node;
> +		}
>   
>   		if (!keyring_ptr_is_keyring(ptr))
>   			continue;

Friendly ping.

Best regards,
Ridong