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 <[email protected]> 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/[email protected]?part=2
