Re: [RFC PATCH 3/4] KEYS: Remove key_type_dead

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KEYS: unregister_key_type() leaves live keys active for runtime callers
- [High] KEYS: UAF in unregister_key_type() when module_init fails after registration
--

commit 5a5ac1aac59bbfa4e21a7a0f1fe3391d7d6ec2c7
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Remove key_type_dead

This commit removes key_type_dead, key_gc_keytype(), and
keyring_restriction_gc(), assuming that pinning the key type's owning module
while keys or restrictions exist prevents unregister_key_type() from being
called while keys of that type are alive.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index 9f1f08fcbe863..c0efb87b6705d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -1279,18 +1279,14 @@ EXPORT_SYMBOL(__register_key_type);
>   * unregister_key_type - Unregister a type of key.
>   * @ktype: The key type.
>   *
> - * Unregister a key type and mark all the extant keys of this type as dead.
> - * Those keys of this type are then destroyed to get rid of their payloads and
> - * they and their links will be garbage collected as soon as possible.
> + * Unregister a key type.
>   */
>  void unregister_key_type(struct key_type *ktype)
>  {
>  	down_write(&key_types_sem);
>  	list_del_init(&ktype->link);
> -	downgrade_write(&key_types_sem);
> -	key_gc_keytype(ktype);
>  	pr_notice("Key type %s unregistered\n", ktype->name);
> -	up_read(&key_types_sem);
> +	up_write(&key_types_sem);
>  }

[Severity: High]
What happens when unregister_key_type() is called at runtime from a device
driver removal callback or error recovery path rather than module unload?

Pinning ktype->owner in key_alloc() and keyring_restrict() prevents
delete_module() from running module_exit, but it does not stop runtime calls
to unregister_key_type() while the module remains loaded or when the key type
is built-in.

For example, in security/keys/trusted-keys/trusted_tee.c, when the parent
driver unbinds and optee_remove_common() -> optee_unregister_devices() ->
device_unregister() invokes trusted_key_remove(), it unregisters
key_type_trusted and immediately frees pvt_data.ctx:

security/keys/trusted-keys/trusted_tee.c:trusted_key_remove() {
	unregister_key_type(&key_type_trusted);
	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
	tee_client_close_context(pvt_data.ctx);
}

Without key_gc_keytype() marking extant keys dead, unlinking them, destroying
their payloads via trusted_destroy(), and retyping them to key_type_dead,
existing trusted keys remain active. If userspace subsequently updates or
instantiates one of those keys via keyctl_update_key() or
keyctl_instantiate_key(), trusted_update() or trusted_instantiate() calls into
trusted_tee_seal(), trusted_tee_unseal(), or trusted_tee_get_random() and
dereferences the freed pvt_data.ctx and pvt_data.dev:

security/keys/trusted-keys/trusted_tee.c:trusted_tee_seal() {
	reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
					      sizeof(p->key) + sizeof(p->blob));
	if (IS_ERR(reg_shm)) {
		dev_err(pvt_data.dev, "shm register failed\n");
		return PTR_ERR(reg_shm);
	}
}

Similarly, in arch/s390/kernel/cert_store.c, where key_type_cert_store_key is
built-in (owner is null), a runtime certificate store refresh via
create_cs_keyring() -> cleanup_cs_keys() relies on unregister_key_type() to
force-reap and destroy extant cert_store_key keys when per-key invalidation
fails before re-registering key_type_cert_store_key:

arch/s390/kernel/cert_store.c:cleanup_cs_keys() {
	if (invalidate_keyring_keys(cs_keyring))
		unregister_key_type(&key_type_cert_store_key);
}

In both paths, if register_key_type() is later called again on the same static
struct key_type (in create_cs_keyring() or trusted_key_probe()) while keys
from the previous registration are still alive, __register_key_type() will
also zero ktype->lock_class while live keys still have key->sem registered
with lockdep using &ktype->lock_class:

security/keys/key.c:__register_key_type() {
	ktype->owner = owner;
	memset(&ktype->lock_class, 0, sizeof(ktype->lock_class));
}

Doesn't unregister_key_type() still need to reap and retype extant keys so
runtime callers cannot leave live keys referencing freed driver resources or
active lockdep classes?

[Severity: High]
Could removing key_gc_keytype() and keyring_restriction_gc() from
unregister_key_type() also cause a use-after-free when a module's init
function fails after calling register_key_type()?

Several loadable modules register key types before performing subsequent
fallible initialization steps and call unregister_key_type() on their init
error unwind path, such as af_rxrpc_init() in net/rxrpc/af_rxrpc.c,
turris_signing_key_init() in drivers/platform/cznic/turris-signing-key.c,
init_nfs_fs() in fs/nfs/inode.c, nfs_idmap_init() in fs/nfs/nfs4idmap.c,
init_cifs() in fs/smb/client/cifsfs.c, and init_ceph_lib() in
net/ceph/ceph_common.c:

drivers/platform/cznic/turris-signing-key.c:turris_signing_key_init() {
	if (IS_ERR(turris_signing_keyring)) {
		pr_err("Cannot allocate Turris keyring\n");

		unregister_key_type(&turris_signing_key_type);

		return PTR_ERR(turris_signing_keyring);
	}
}

While mod->init() is running in do_init_module() in kernel/module/main.c,
module_is_live(mod) is true, so try_module_get(type->owner) in key_alloc() and
keyring_restrict() succeeds as soon as register_key_type() publishes the key
type. If a concurrent task allocates a key or restricts a keyring with that
key type and a later step in mod->init() fails, the error path calls
unregister_key_type() and do_init_module() unconditionally frees the module
regardless of its reference count:

kernel/module/main.c:do_init_module() {
fail:
	/* Try to protect us from buggy refcounters. */
	mod->state = MODULE_STATE_GOING;
	synchronize_rcu();
	module_put(mod);
	blocking_notifier_call_chain(&module_notify_list,
				     MODULE_STATE_GOING, mod);
	klp_module_going(mod);
	ftrace_release_mod(mod);
	free_module(mod);
}

Previously, key_gc_keytype() synchronously destroyed key payloads, retyped
those keys to key_type_dead, and cleared keyring restrictions via
keyring_restriction_gc() before unregister_key_type() returned. Without that
cleanup, won't surviving keys and restrictions retain dangling pointers into
freed module memory when later accessed or garbage-collected in
key_gc_unused_keys() in security/keys/gc.c and keyring_destroy() in
security/keys/keyring.c:

security/keys/gc.c:key_gc_unused_keys() {
		struct key *key =
			list_entry(keys->next, struct key, graveyard_link);
		struct module *owner = key->type->owner;
		short state = key->state;
		...
		if (state == KEY_IS_POSITIVE && key->type->destroy)
			key->type->destroy(key);
		...
		module_put(owner);
}

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