Re: [PATCH v13 6/7] KEYS: Replace assoc_array with rhashtable

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

> KEYS: Replace assoc_array with rhashtable

TL;DR: Comment only, not a bug today: the new keyring_read() comment says no
semaphore is needed and concurrent changes are possible, but the caller's
semaphore prevents that. Suggests rewording it.

> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c

[ ... ]

>  /*
>   * Read a list of key IDs from the keyring's contents in binary form
>   *
> - * The keyring's semaphore is read-locked by the caller.  This prevents someone
> - * from modifying it under us - which could cause us to read key IDs multiple
> - * times.
> + * The walk is RCU-protected, so the keyring's semaphore is not required.  A
> + * keyring modified concurrently may list a key twice or miss one, but it can
> + * never have a link freed under the walk.
>   */
>  static long keyring_read(const struct key *keyring,
>  			 char *buffer, size_t buflen)

[ ... ]

> +		ret = keyring_iterate(keyring, keyring_read_iterator, &ctx);
> +		if (ret < 0)
>  			return ret;

[ ... ]

This isn't a bug today, but the new comment looks inaccurate.  It says the
keyring's semaphore is not required and that a concurrent modification may
list a key twice or miss one.

keyring_read() walks the list under RCU, but the length it returns is
computed separately afterwards:

    ret = keyring_iterate(keyring, keyring_read_iterator, &ctx);
    ...
    ret = keyring_nr_links(keyring) * sizeof(key_serial_t);

keyctl_read_key() allocates key_data with kvmalloc(), which is not zeroed,
and when the returned length is <= buflen it does:

    ret = __keyctl_read_key(key, key_data, key_data_len);
    if (ret <= 0 || ret > buflen)
        break;
    ...
    if (copy_to_user(buffer, key_data, ret))

If a link were added between the walk and keyring_nr_links(), ret would
cover more entries than were written, and the extra uninitialised bytes
would be copied to userspace.

Today this can't happen, because __keyctl_read_key() does
down_read(&key->sem) (as does the s390 invalidate_keyring_keys()), and every
writer of index->list and index->nr (__key_link_lock(), __key_move_lock(),
__key_unlink_lock(), keyring_clear(), keyring_gc(), and key_revoke() via
keyring_revoke()) holds the semaphore for write.  So the concurrent
modification described by the comment does not occur.

Could the comment instead say that the caller holds the keyring's semaphore,
which keeps the list and the link count stable across the walk and the
length calculation, with RCU only keeping the walk itself memory-safe?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/43634583011