Re: [PATCH] KEYS: Protect pkey method checks against type removal

[Jarkko Sakkinen]

On Tue, Nov 03, 2026 at 11:29:40AM +0800, BeaCox wrote:
> keyctl_pkey_params_get() looks up the key, and keyctl_pkey_query(),
> keyctl_pkey_e_d_s() and keyctl_pkey_verify() then dereference
> key->type->asym_* outside key->sem.  A reference keeps the key alive, but
> does not pin the module providing its type.
> 
> unregister_key_type() calls key_gc_keytype(), which retypes every
> remaining key of the type to key_type_dead under key->sem for writing.
> The pkey paths read key->type again after their checks, so a concurrent
> module unload can turn the checked callback into the NULL key_type_dead
> member and call through a NULL function pointer, e.g.:
> 
>   KEYCTL_PKEY_SIGN task              Module unload and key GC
>   ------------------------           ------------------------
>   keyctl_pkey_e_d_s()
>     if (!key->type->asym_eds_op)     unregister_key_type()
>                                        GC: down_write(&key->sem)
>                                        GC: retype to key_type_dead
>                                        GC: up_write(&key->sem)
>                                      free_module()
>     in = memdup_user(...)
>     key->type->asym_eds_op(...)      -> NULL call
> 
> In-tree, turris-signing-key provides such methods; out-of-tree asymmetric
> key types are affected as well.

OOT-whatever should be knowingly ignored. We do not care about that
side.

> 
> Hold key->sem for reading from the lookup until the parameter block is
> freed, so the type cannot be retired between the method check and the
> matching call.
> 
> This is the same type-removal problem recently fixed for the read
> (925c84e3cc21) and update (aef3a8a4f988) methods.

This is not as this code change effectively changes ownership of the
semaphore, which is not a great idea.

> 
> Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: BeaCox <0xbeac@xxxxxxxxx>
> ---
>  security/keys/keyctl_pkey.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6..b7e3648 100644
> --- a/security/keys/keyctl_pkey.c
> +++ b/security/keys/keyctl_pkey.c
> @@ -17,7 +17,10 @@
>  static void keyctl_pkey_params_free(struct kernel_pkey_params *params)
>  {
>  	kfree(params->info);
> -	key_put(params->key);
> +	if (params->key) {
> +		up_read(&params->key->sem);
> +		key_put(params->key);
> +	}
>  }
>  
>  enum {
> @@ -100,6 +103,7 @@ static int keyctl_pkey_params_get(key_serial_t id,
>  		return PTR_ERR(key_ref);
>  	params->key = key_ref_to_ptr(key_ref);
>  
> +	down_read(&params->key->sem);
>  	if (!params->key->type->asym_query)
>  		return -EOPNOTSUPP;
>  
> 

Br, Jarkko