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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, 2024-10-16 at 09:33 +0800, Chen Ridong wrote:
> 
> 
> 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.

Thanks for pinging because I actually accidentally missed the original
email!

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

I'll pick this up.

> 
> Best regards,
> Ridong
> 
> 

BR, Jarkko