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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Nov 05, 2026 at 12:38:21PM +0800, Yuansheng Yang 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.
> 

Look at this:

> Hold key->sem for reading around the method check and the matching call,
> so that the type cannot be retired in between.  Each keyctl handler takes
> and releases the lock itself; the semaphore is not passed out through the
> parameter block.  User buffers are copied outside the critical section.
> 
> This is the same type-removal problem recently fixed for the read
> (925c84e3cc21) and update (aef3a8a4f988) methods.

I don't think we should be making up random English words for
well-defined objects.

The first paragraph in this should be replaced with the description how
keyctl_pkey_params_get and other functions gets modified. I.e., how and
why they get modified.

Also "user buffers", "the semaphore" etc. are hallucinated words with no
actual meaning. Maybe it is just me but I do personally think that
key->sem is a better description of key->sem than "the semaphore".

In the second paragraph I would instead:

	This problem was recently fixed for 925c84e3cc21 ("keys: Protect the
	read method check against type removal") and ...

The commit message should be useful after the patch has been applied. I.e.
when a bug is bisected it is useful to check from the commit message what was
done.

The commit message is as import as the code change itself. With a random code
change you are 50% done.

> 
> Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuansheng Yang <0xbeac@gmail.com>
> ---
> Changes in v3:
> - Keep the switch, allocations and existing error handling in place.
> - Use one unlock point per method check/call pair.
> - Set -EOPNOTSUPP inside the failed checks; add blank lines before labels.
> - Use my real name in the author and sign-off fields.
> 
> v2: https://lore.kernel.org/all/20261104021414.50696-1-0xbeac@gmail.com/

This makes much more sense than the previous version.

> 
>  security/keys/keyctl_pkey.c | 47 ++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399..40475817dd 100644
> --- a/security/keys/keyctl_pkey.c
> +++ b/security/keys/keyctl_pkey.c
> @@ -100,9 +100,6 @@ static int keyctl_pkey_params_get(key_serial_t id,
>  		return PTR_ERR(key_ref);
>  	params->key = key_ref_to_ptr(key_ref);
>  
> -	if (!params->key->type->asym_query)
> -		return -EOPNOTSUPP;
> -
>  	return 0;
>  }
>  
> @@ -129,7 +126,16 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
>  	if (ret < 0)
>  		return ret;
>  
> +	down_read(&params->key->sem);
> +	if (!params->key->type->asym_query) {
> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
>  	ret = params->key->type->asym_query(params, &info);
> +
> +out_unlock:

out_up_key_sem:

> +	up_read(&params->key->sem);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -185,7 +191,16 @@ long keyctl_pkey_query(key_serial_t id,
>  	if (ret < 0)
>  		goto error;
>  
> +	down_read(&params.key->sem);
> +	if (!params.key->type->asym_query) {
> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
>  	ret = params.key->type->asym_query(&params, &res);
> +
> +out_unlock:

out_up_key_sem:

> +	up_read(&params.key->sem);
>  	if (ret < 0)
>  		goto error;
>  
> @@ -230,10 +245,6 @@ long keyctl_pkey_e_d_s(int op,
>  	if (ret < 0)
>  		goto error_params;
>  
> -	ret = -EOPNOTSUPP;
> -	if (!params.key->type->asym_eds_op)
> -		goto error_params;
> -
>  	switch (op) {
>  	case KEYCTL_PKEY_ENCRYPT:
>  		params.op = kernel_pkey_encrypt;
> @@ -260,7 +271,16 @@ long keyctl_pkey_e_d_s(int op,
>  	if (!out)
>  		goto error_in;
>  
> +	down_read(&params.key->sem);
> +	if (!params.key->type->asym_eds_op) {
> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
>  	ret = params.key->type->asym_eds_op(&params, in, out);
> +
> +out_unlock:

out_up_key_sem:

> +	up_read(&params.key->sem);
>  	if (ret < 0)
>  		goto error_out;
>  
> @@ -307,10 +327,6 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
>  	if (ret < 0)
>  		goto error_params;
>  
> -	ret = -EOPNOTSUPP;
> -	if (!params.key->type->asym_verify_signature)
> -		goto error_params;
> -
>  	in = memdup_user(_in, params.in_len);
>  	if (IS_ERR(in)) {
>  		ret = PTR_ERR(in);
> @@ -324,8 +340,17 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
>  	}
>  
>  	params.op = kernel_pkey_verify;
> +	down_read(&params.key->sem);
> +	if (!params.key->type->asym_verify_signature) {
> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
>  	ret = params.key->type->asym_verify_signature(&params, in, in2);
>  
> +out_unlock:

out_up_key_sem:

> +	up_read(&params.key->sem);
> +
>  	kfree(in2);
>  error_in:
>  	kfree(in);

Exactly at this site I would do just little bit more hygiene work:

	out_up_key_sem
		/* ... */

	out_free_in:
		/* ... */

	out_free_params:
		/* ... */

	/* ... */

This adheres with the  https://docs.kernel.org/process/coding-style.html
section conventions.

At sites having already out_-labels I would not do extra renaming.

> -- 
> 2.43.0
> 

Br, Jarkko