Re: [RFC PATCH 2/4] KEYS: Pin the backing module for struct key_type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KEYS: self-pinning module reference cycle in key_alloc()
--

commit 8793dbc22dd9f6067e0bfcfa9c4b1b437edcf052
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Pin the backing module for struct key_type

This commit adds an owner field populated with THIS_MODULE to struct key_type
when registering a key type. It pins the module in key_alloc() and
keyring_restrict(), releasing the reference in key_gc_unused_keys() and
keyring_destroy().

> diff --git a/security/keys/key.c b/security/keys/key.c
> index e88548f1a7040..9f1f08fcbe863 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/export.h>
>  #include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/poison.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -234,19 +235,24 @@ struct key *key_alloc(struct key_type *type, const char *desc,
>  
>  	key = ERR_PTR(-EINVAL);
>  	if (!desc || !*desc)
> -		goto error;
> +		goto out;
> +
> +	if (!try_module_get(type->owner)) {
> +		key = ERR_PTR(-ENOENT);
> +		goto out;
> +	}

[Severity: High]
Does pinning type->owner in key_alloc() until key_gc_unused_keys() create a
self-pinning reference cycle for modular key types that cache keys in their
own internal keyrings?

With register_key_type() setting ktype->owner to THIS_MODULE:

include/linux/key-type.h:
#define register_key_type(ktype) __register_key_type(ktype, THIS_MODULE)

every allocated key of that type holds a module reference on its owning
module until the key's reference count drops to zero and it is garbage
collected in key_gc_unused_keys().

However, several loadable modules allocate an internal keyring during module
initialization, link keys of their own key_type into that keyring during
normal operation, and only revoke and release that keyring in their module
exit path:

drivers/nvme/common/keyring.c:nvme_keyring_exit() {
	unregister_key_type(&nvme_tls_psk_key_type);
	key_revoke(nvme_keyring);
	key_put(nvme_keyring);
}

fs/nfs/nfs4idmap.c:nfs_idmap_quit() {
	key_revoke(id_resolver_cache->thread_keyring);
	unregister_key_type(&key_type_id_resolver);
	unregister_key_type(&key_type_id_resolver_legacy);
	put_cred(id_resolver_cache);
}

fs/smb/client/cifsacl.c:exit_cifs_idmap() {
	key_revoke(root_cred->thread_keyring);
	unregister_key_type(&cifs_idmap_key_type);
	put_cred(root_cred);
	cifs_dbg(FYI, "Unregistered %s key type\n", cifs_idmap_key_type.name);
}

net/dns_resolver/dns_key.c:exit_dns_resolver() {
	key_revoke(dns_resolver_cache->thread_keyring);
	unregister_key_type(&key_type_dns_resolver);
	put_cred(dns_resolver_cache);
}

Once any key is linked into one of these internal module keyrings (or added
by userspace via add_key() to a session or user keyring), the key holds a
reference on the module while the module's keyring holds a reference on the
key. Won't this prevent the module reference count from ever reaching zero,
making it impossible to unload these modules unless all cached keys expire or
are manually evicted first?

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