Re: [PATCH v8 2/3] KEYS: Replace assoc_array with rhashtable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] keys: uninitialized return value in __key_link_begin() on
  link replacement
- [Critical] keys: goto jumps past __free(kfree) initialization in
  __key_link_begin()
--

commit 1bfb81a86d8ef725610c02d9a07de1a279485dd1
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Replace assoc_array with rhashtable

This commit replaces struct assoc_array in struct key with a new
rhashtable- and list-backed struct keyring_index and struct keyring_link
implementation.

> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index ac48d6204915a..60309f18e3a56 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c

[ ... ]

> @@ -1317,57 +1155,94 @@ int __key_move_lock(struct key *l_keyring, struct key *u_keyring,
>  int __key_link_begin(struct key *keyring,
>  		     const struct keyring_index_key *index_key,
> -		     struct assoc_array_edit **_edit)
> +		     struct keyring_edit *edit)
>  {
> -	struct assoc_array_edit *edit;
> +	struct keyring_link *replace;
> +	struct keyring_index *links;
>  	int ret;
>  
>  	kenter("%d,%s,%s,",
>  	       keyring->serial, index_key->type->name, index_key->description);
>  
> -	BUG_ON(index_key->desc_len == 0);
> -	BUG_ON(*_edit != NULL);
> +	if (WARN_ON(index_key->desc_len == 0) || WARN_ON(edit->new)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
> -	*_edit = NULL;
> +	edit->new = NULL;
> +	edit->old = NULL;
>  
> -	ret = -EKEYREVOKED;
> -	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
> -		goto error;
> +	if (test_bit(KEY_FLAG_REVOKED, &keyring->flags)) {
> +		ret = -EKEYREVOKED;
> +		goto out;
> +	}
>  
> -	/* Create an edit script that will insert/replace the key in the
> -	 * keyring tree.
> -	 */
> -	edit = assoc_array_insert(&keyring->keys,
> -				  &keyring_assoc_array_ops,
> -				  index_key,
> -				  NULL);
> -	if (IS_ERR(edit)) {
> -		ret = PTR_ERR(edit);
> -		goto error;
> +	links = keyring_get_index(keyring);
> +	if (IS_ERR(links)) {
> +		ret = PTR_ERR(links);
> +		goto out;
>  	}
>  
> -	/* If we're not replacing a link in-place then we're going to need some
> +	struct keyring_link *link __free(kfree) = kzalloc_obj(struct keyring_link);

[Severity: Critical]
Do the three goto out statements above jump past the initialization of
link __free(kfree) into its scope?

This breaks the build with Clang (and GCC with -Werror=jump-misses-init),
and without stack auto-initialization, linking a key into a revoked
keyring jumps to out and runs kfree(link) on an uninitialized stack
pointer.  Returning early instead of jumping to out avoids entering the
cleanup scope before link is initialized.

> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	link->hash = index_key->hash;
> +
> +	replace = keyring_find_link(links, index_key);
> +
> +	/*
> +	 * If we're not replacing a link in-place then we're going to need some
>  	 * extra quota.
>  	 */
> -	if (!edit->dead_leaf) {
> +	if (!replace) {
>  		ret = key_payload_reserve(keyring,
>  					  keyring->datalen + KEYQUOTA_LINK_BYTES);
>  		if (ret < 0)
> -			goto error_cancel;
> +			goto out;
> +
> +		ret = rhashtable_insert_fast(&links->ht, &link->node,
> +					     keyring_link_params);
> +		if (ret < 0) {
> +			key_payload_reserve(keyring,
> +					    keyring->datalen - KEYQUOTA_LINK_BYTES);
> +			goto out;
> +		}
>  	}
>  
> -	*_edit = edit;
> -	kleave(" = 0");
> -	return 0;
> +	edit->new = no_free_ptr(link);
> +	edit->old = replace;
>  
> -error_cancel:
> -	assoc_array_cancel_edit(edit);
> -error:
> +out:
>  	kleave(" = %d", ret);
>  	return ret;
>  }

[Severity: Critical]
Does __key_link_begin() return an uninitialized ret when replace is
non-NULL?

When a matching key already exists in the keyring, the if (!replace)
block is skipped without setting ret = 0.  In __key_create_or_update(), a
negative stack value below -MAX_ERRNO makes ERR_PTR(ret) return a
non-error pointer that add_key() dereferences and passes to
key_ref_put().  In key_reject_and_link(), a positive stack value skips
both the link_ret < 0 and link_ret == 0 paths, leaking edit.new and
returning with keyring->sem write-locked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110044043.2608600-1-jarkko@kernel.org?part=2