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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Nov 10, 2026 at 11:02:52AM +0800, Yuansheng Yang wrote:
> keyctl_pkey_query(), keyctl_pkey_e_d_s() and keyctl_pkey_verify() check
> that key->type has the ->asym_*() method they need and call it later,
> without holding key->sem.  A reference keeps the key alive, but does not
> pin the module that provides its type.
> 
> When that module is unloaded, unregister_key_type() waits for the key GC
> to process every remaining key of the type.  For each such key, the GC
> takes key->sem for writing, switches key->type to key_type_dead, whose
> ->asym_*() methods are NULL, and destroys the payload.  If that happens
> between the check and the call, the keyctl calls a NULL method; if it
> happens while the method runs, the method returns into freed module text.
> In-tree, turris-signing-key provides ->asym_query() and ->asym_eds_op()
> and can be built as a module.
> 
> Take key->sem for reading around each method call, and check the method
> again under it, right before the call.  While the lock is held the GC
> cannot switch the key to key_type_dead, so unregister_key_type() and the
> module unload cannot complete until the method returns.  If the type is
> already gone, the check finds key_type_dead and the keyctl fails with
> -EOPNOTSUPP, as it does for any key type without the method.
> 
> memdup_user() and copy_to_user() stay outside key->sem, as in
> keyctl_read_key(), so no page fault is taken with key->sem held.  The
> in-tree ->asym_*() methods do not take key->sem themselves.  Nothing else
> in the kernel calls these methods, so these four call sites are the full
> coverage:
> 
>  - keyctl_pkey_query(): ->asym_query() for KEYCTL_PKEY_QUERY
>  - keyctl_pkey_params_get_2(): ->asym_query(), and the op-specific check
>    for KEYCTL_PKEY_ENCRYPT, _DECRYPT, _SIGN and _VERIFY
>  - keyctl_pkey_e_d_s(): ->asym_eds_op() for _ENCRYPT, _DECRYPT and _SIGN
>  - keyctl_pkey_verify(): ->asym_verify_signature() for _VERIFY
> 
> This problem was recently fixed for keyctl_read_key() by 925c84e3cc21
> ("keys: Protect the read method check against type removal") and for
> key_update() by aef3a8a4f988 ("KEYS: Protect the update method check
> against type removal").
> 
> 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 v7:
> - Restore the early ->asym_query() check in keyctl_pkey_params_get().
>   Removing it was not part of the bug fix (Jarkko).  The function is now
>   identical to 7.4-rc1 again, so the diff does not touch it at all.
>   Sashiko flagged that unlocked read on v5; it is pre-existing code and
>   is left alone here, as asked.  The locked rechecks at the call sites
>   are what the fix relies on.
> - Make the commit message more compact (Jarkko).
> 
> Changes in v6:
> - Address the Sashiko review of v5: keyctl_pkey_e_d_s() and
>   keyctl_pkey_verify() still read a ->asym_*() method pointer without
>   key->sem in their early checks.  Move those op-specific checks into
>   keyctl_pkey_params_get_2(), in the same key->sem critical section that
>   calls ->asym_query(), and after the buffer size validation, so a key
>   type without the method still fails with -EOPNOTSUPP before
>   memdup_user() and the error precedence is unchanged.
> - Drop the LLM paragraph from the commit message (Jarkko).
> 
> Tested on x86_64 under QEMU/KVM, with KASAN, lockdep and
> DEBUG_ATOMIC_SLEEP, on 7.4-rc1 with and without this patch.  The
> test-only key type module is not for merging.
> 
>   race_sign, race_verify: the module is unloaded while the keyctl waits
>   in memdup_user() after the type check.  Without the patch: NULL call
>   from keyctl_pkey_e_d_s() / keyctl_pkey_verify().  With the patch: both
>   return -EOPNOTSUPP and delete_module() has returned first.
> 
>   inop: the module's ->asym_eds_op() sleeps in module text while the
>   module is unloaded.  Without the patch: return into freed module text.
>   With the patch: delete_module() returns only after the method has
>   returned (about 3.1 s).
> 
>   func_asym, func_fake: KEYCTL_PKEY_QUERY, _ENCRYPT, _DECRYPT, _SIGN and
>   _VERIFY on a real RSA-2048 asymmetric key and on the test type; the
>   CASE lines are identical with and without the patch.
> 
>   errno: a type with ->asym_query() but no ->asym_verify_signature()
>   returns -EOPNOTSUPP before reading the input; a request with an
>   oversized length still returns -EINVAL first, as before.  The CASE
>   lines are identical with and without the patch.
> 
> In the forced races lockdep can report "bad unlock balance" for
> key->sem.  Its lock class is the type's lock_class, which lockdep drops
> when the module is freed, so any key->sem user on a key whose type was
> unregistered can hit this, e.g. keyctl_read_key().  KASAN reports
> nothing.  Not tested on Turris hardware.
> 
> v6: https://lore.kernel.org/all/20261109055204.31673-1-0xbeac@gmail.com/
> v5: https://lore.kernel.org/all/20261108131417.77287-1-0xbeac@gmail.com/
> v4: https://lore.kernel.org/all/20261105193632.34363-1-0xbeac@gmail.com/
> v3: https://lore.kernel.org/all/9d47e202bd11857b3555e6b95694b3c3a4f2460b.1793852399.git.0xbeac@gmail.com/
> v2: https://lore.kernel.org/all/20261104021414.50696-1-0xbeac@gmail.com/
> v1: https://lore.kernel.org/all/20261103032940.51826-1-0xbeac@gmail.com/
> 
>  security/keys/keyctl_pkey.c | 100 +++++++++++++++++++++++++++---------
>  1 file changed, 76 insertions(+), 24 deletions(-)
> 
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399..e7121523b9 100644
> --- a/security/keys/keyctl_pkey.c
> +++ b/security/keys/keyctl_pkey.c
> @@ -129,45 +129,77 @@ 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_up_key_sem;
> +	}
> +
>  	ret = params->key->type->asym_query(params, &info);
>  	if (ret < 0)
> -		return ret;
> +		goto out_up_key_sem;
>  
>  	switch (op) {
>  	case KEYCTL_PKEY_ENCRYPT:
>  		if (uparams.in_len  > info.max_dec_size ||
> -		    uparams.out_len > info.max_enc_size)
> -			return -EINVAL;
> +		    uparams.out_len > info.max_enc_size) {
> +			ret = -EINVAL;
> +			goto out_up_key_sem;
> +		}
>  
>  		params->out_len = info.max_enc_size;
>  		break;
>  	case KEYCTL_PKEY_DECRYPT:
>  		if (uparams.in_len  > info.max_enc_size ||
> -		    uparams.out_len > info.max_dec_size)
> -			return -EINVAL;
> +		    uparams.out_len > info.max_dec_size) {
> +			ret = -EINVAL;
> +			goto out_up_key_sem;
> +		}
>  
>  		params->out_len = info.max_dec_size;
>  		break;
>  	case KEYCTL_PKEY_SIGN:
>  		if (uparams.in_len  > info.max_data_size ||
> -		    uparams.out_len > info.max_sig_size)
> -			return -EINVAL;
> +		    uparams.out_len > info.max_sig_size) {
> +			ret = -EINVAL;
> +			goto out_up_key_sem;
> +		}
>  
>  		params->out_len = info.max_sig_size;
>  		break;
>  	case KEYCTL_PKEY_VERIFY:
>  		if (uparams.in_len  > info.max_data_size ||
> -		    uparams.in2_len > info.max_sig_size)
> -			return -EINVAL;
> +		    uparams.in2_len > info.max_sig_size) {
> +			ret = -EINVAL;
> +			goto out_up_key_sem;
> +		}
>  
>  		params->out_len = info.max_sig_size;
>  		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto out_up_key_sem;
>  	}
>  
>  	params->in_len  = uparams.in_len;
> -	return 0;
> +
> +	switch (op) {
> +	case KEYCTL_PKEY_VERIFY:
> +		if (!params->key->type->asym_verify_signature)
> +			ret = -EOPNOTSUPP;
> +		break;
> +	case KEYCTL_PKEY_ENCRYPT:
> +	case KEYCTL_PKEY_DECRYPT:
> +	case KEYCTL_PKEY_SIGN:
> +		if (!params->key->type->asym_eds_op)
> +			ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +out_up_key_sem:
> +	up_read(&params->key->sem);
> +	return ret;
>  }
>  
>  /*
> @@ -185,7 +217,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_up_key_sem;
> +	}
> +
>  	ret = params.key->type->asym_query(&params, &res);
> +
> +out_up_key_sem:
> +	up_read(&params.key->sem);
>  	if (ret < 0)
>  		goto error;
>  
> @@ -230,10 +271,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 +297,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_up_key_sem;
> +	}
> +
>  	ret = params.key->type->asym_eds_op(&params, in, out);
> +
> +out_up_key_sem:
> +	up_read(&params.key->sem);
>  	if (ret < 0)
>  		goto error_out;
>  
> @@ -305,31 +351,37 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
>  	ret = keyctl_pkey_params_get_2(_params, _info, KEYCTL_PKEY_VERIFY,
>  				       &params);
>  	if (ret < 0)
> -		goto error_params;
> -
> -	ret = -EOPNOTSUPP;
> -	if (!params.key->type->asym_verify_signature)
> -		goto error_params;
> +		goto out_free_params;
>  
>  	in = memdup_user(_in, params.in_len);
>  	if (IS_ERR(in)) {
>  		ret = PTR_ERR(in);
> -		goto error_params;
> +		goto out_free_params;
>  	}
>  
>  	in2 = memdup_user(_in2, params.in2_len);
>  	if (IS_ERR(in2)) {
>  		ret = PTR_ERR(in2);
> -		goto error_in;
> +		goto out_free_in;
>  	}
>  
>  	params.op = kernel_pkey_verify;
> +	down_read(&params.key->sem);
> +	if (!params.key->type->asym_verify_signature) {
> +		ret = -EOPNOTSUPP;
> +		goto out_up_key_sem;
> +	}
> +
>  	ret = params.key->type->asym_verify_signature(&params, in, in2);
>  
> +out_up_key_sem:
> +	up_read(&params.key->sem);
>  	kfree(in2);
> -error_in:
> +
> +out_free_in:
>  	kfree(in);
> -error_params:
> +
> +out_free_params:
>  	keyctl_pkey_params_free(&params);
>  	return ret;
>  }
> -- 
> 2.43.0
> 

Thanks for quick response.


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko