Re: [PATCH v11 5/6] KEYS: Replace assoc_array with rhashtable

[bot+bpf-ci@kernel.org]

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

> KEYS: Replace assoc_array with rhashtable

TL;DR: Not a bug today, but the new keyring_read() comment says the semaphore
isn't needed; without it, a missed key could possibly leak uninitialized
kvmalloc() bytes to user space. Comment should say the read lock matters.

> 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)

This isn't a bug today, but is the new comment accurate about the
semaphore not being required?

keyring_read() writes serials while walking the list, and then computes the
returned length separately from the link count:

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

keyctl_read_key() kvmalloc()s key_data without zeroing it and copies back
exactly ret bytes when ret <= buflen:

    key_data = kvmalloc(key_data_len, GFP_KERNEL);
    ret = __keyctl_read_key(key, key_data, key_data_len);
    ...
    if (copy_to_user(buffer, key_data, ret))

Both callers of ->read, __keyctl_read_key() and read_sed_opal_key(), hold
down_read(&key->sem), and every writer of index->list and index->nr
(__key_link(), __key_unlink(), keyring_gc(), keyring_clear(),
keyring_revoke()) holds down_write() on the same semaphore, so the walk
always writes exactly nr serials.

Without the semaphore, a walk that misses a key, as the comment allows
(for example a link prepended with list_add_rcu() after the walk has passed
the head), would leave keyring_nr_links() larger than the number of serials
written.  Could the uninitialized tail of the kvmalloc() buffer then be
copied to user space?

The old comment described the requirement correctly.  Could the new comment
say that the caller's read lock on the semaphore is what keeps the returned
length consistent with the serials written?


---
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/43615900108