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

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

[Yuansheng Yang]

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.

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.

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/

 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:
+	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:
+	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:
+	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:
+	up_read(&params.key->sem);
+
 	kfree(in2);
 error_in:
 	kfree(in);
-- 
2.43.0

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

[Jarkko Sakkinen]

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

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

[Yuansheng Yang]

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.

Remove the key->type->asym_query check from keyctl_pkey_params_get(): it
runs without key->sem, so key_gc_keytype() can retype the key right after
it.  Instead, make keyctl_pkey_params_get_2(), keyctl_pkey_query(),
keyctl_pkey_e_d_s() and keyctl_pkey_verify() each take key->sem for
reading, check the key->type->asym_* method they are about to call, call
it and release key->sem.  As key_gc_keytype() takes key->sem for writing
before it retypes the key, key->type cannot change between the check and
the call.  Drop the asym_eds_op and asym_verify_signature checks that
keyctl_pkey_e_d_s() and keyctl_pkey_verify() did before memdup_user() for
the same reason; -EOPNOTSUPP is now returned from under key->sem.
memdup_user(), kmalloc() and copy_to_user() stay outside key->sem, as in
__keyctl_read_key().

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 v4:
- Rewrite the commit message: describe what changes in
  keyctl_pkey_params_get(), keyctl_pkey_params_get_2() and the three
  keyctl handlers and why, and cite the read/update fixes by subject.
- Rename the out_unlock labels to out_up_key_sem.
- Use out_up_key_sem / out_free_in / out_free_params in
  keyctl_pkey_verify(); leave the other functions' existing labels alone.
- No functional change relative to v3.

v3: https://lore.kernel.org/all/9d47e202bd11857b3555e6b95694b3c3a4f2460b.1793852399.git.0xbeac@gmail.com/
v2: https://lore.kernel.org/all/20261104021414.50696-1-0xbeac@gmail.com/

 security/keys/keyctl_pkey.c | 58 +++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 16 deletions(-)

diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 15b6bf6399..3e3253330c 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_up_key_sem;
+	}
+
 	ret = params->key->type->asym_query(params, &info);
+
+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_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 +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_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 +325,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

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

[Jarkko Sakkinen]

On Fri, Nov 06, 2026 at 03:36:31AM +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.
> 
> Remove the key->type->asym_query check from keyctl_pkey_params_get(): it
> runs without key->sem, so key_gc_keytype() can retype the key right after
> it.  Instead, make keyctl_pkey_params_get_2(), keyctl_pkey_query(),
> keyctl_pkey_e_d_s() and keyctl_pkey_verify() each take key->sem for
> reading, check the key->type->asym_* method they are about to call, call
> it and release key->sem.  As key_gc_keytype() takes key->sem for writing
> before it retypes the key, key->type cannot change between the check and
> the call.  Drop the asym_eds_op and asym_verify_signature checks that
> keyctl_pkey_e_d_s() and keyctl_pkey_verify() did before memdup_user() for
> the same reason; -EOPNOTSUPP is now returned from under key->sem.
> memdup_user(), kmalloc() and copy_to_user() stay outside key->sem, as in
> __keyctl_read_key().

This is a transliteration of the source code changes. How this helps me
to quickly understand them vs reading the source code changes and 
skipping this?

I'd like to know what is that you are trying to do by doing those
changes, which is the higher leve question. You should know this
already. I see pretty much the same thing done for those sites so maybe
that action is what you want to describe. Action taken, why that action
won't cause any harm,  enumerate the coverage and address that it is the
full coverage.

The above looks to me as if someone had literally prompted "translate
this source code change to English". Neither much effort thinking  done.

> 
> 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 v4:
> - Rewrite the commit message: describe what changes in
>   keyctl_pkey_params_get(), keyctl_pkey_params_get_2() and the three
>   keyctl handlers and why, and cite the read/update fixes by subject.
> - Rename the out_unlock labels to out_up_key_sem.
> - Use out_up_key_sem / out_free_in / out_free_params in
>   keyctl_pkey_verify(); leave the other functions' existing labels alone.
> - No functional change relative to v3.
> 
> v3: https://lore.kernel.org/all/9d47e202bd11857b3555e6b95694b3c3a4f2460b.1793852399.git.0xbeac@gmail.com/
> v2: https://lore.kernel.org/all/20261104021414.50696-1-0xbeac@gmail.com/
> 
>  security/keys/keyctl_pkey.c | 58 +++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399..3e3253330c 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;
>  }

I'd drop this as technically it is not part of the bug fix and then you
have pretty much symmetric action. Helps at least with the description.

>  
> @@ -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_up_key_sem;
> +	}
> +
>  	ret = params->key->type->asym_query(params, &info);
> +
> +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_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 +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_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 +325,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
> 

Br, Jarkko

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

[Yuansheng Yang]

On Fri, Nov 06, 2026 at 04:08:15PM +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 06, 2026 at 03:36:31AM +0800, Yuansheng Yang wrote:
> > Remove the key->type->asym_query check from keyctl_pkey_params_get(): it
> > runs without key->sem, so key_gc_keytype() can retype the key right after
> > it.  Instead, make keyctl_pkey_params_get_2(), keyctl_pkey_query(),
> > keyctl_pkey_e_d_s() and keyctl_pkey_verify() each take key->sem for
> > reading, check the key->type->asym_* method they are about to call, call
> > it and release key->sem.
[...]
>
> This is a transliteration of the source code changes. How this helps me
> to quickly understand them vs reading the source code changes and
> skipping this?

It doesn't. It just repeats the diff.

> I'd like to know what is that you are trying to do by doing those
> changes, which is the higher leve question. You should know this
> already. I see pretty much the same thing done for those sites so maybe
> that action is what you want to describe. Action taken, why that action
> won't cause any harm,  enumerate the coverage and address that it is the
> full coverage.

The goal is that an ->asym_*() method is only called while the key's
type cannot go away. The action is the same at every site: take key->sem
for reading, check the method, call it, drop key->sem. The GC needs
key->sem for writing to switch the key to key_type_dead, and
unregister_key_type() waits for the GC, so the type and its module stay
until the method returns.

Why it does no harm: the early checks stay, so the errors do not change
unless the type goes away at the same time. memdup_user() and
copy_to_user() stay outside key->sem. And key->sem is only taken for
reading, so only writers on that key wait for the operation, as they
already wait for ->read().

Coverage: nothing calls ->asym_query(), ->asym_eds_op() or
->asym_verify_signature() except keyctl_pkey_query(),
keyctl_pkey_params_get_2(), keyctl_pkey_e_d_s() and
keyctl_pkey_verify(), and all four get the change.

v5 describes it in that order.

> The above looks to me as if someone had literally prompted "translate
> this source code change to English". Neither much effort thinking  done.

That is close to what happened. I used LLM assistants (Codex and
Claude) to find this bug, while looking for other cases of the
KEYCTL_READ problem, and to draft the patch and its changelogs. I
should have said so from v1. The v5 changelog says so, and what they
were used for. I have gone through this description myself and can
answer for every line of it.

> > -	if (!params->key->type->asym_query)
> > -		return -EOPNOTSUPP;
> > -
>
> I'd drop this as technically it is not part of the bug fix and then you
> have pretty much symmetric action. Helps at least with the description.

Agreed. v5 keeps that check. For the same reason it also keeps the early
asym_eds_op and asym_verify_signature checks in keyctl_pkey_e_d_s() and
keyctl_pkey_verify(), so each of the four sites only gets the
down_read() / check / call / up_read() block.

Keeping them also fixes something I missed in v4: for a key type without
->asym_verify_signature, such as turris-signing-key, KEYCTL_PKEY_VERIFY
with a bad input pointer returned -EFAULT instead of -EOPNOTSUPP.

I'll send v5 right after this mail.

Thanks,
Yuansheng Yang

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

[Yuansheng Yang]

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 this happens
between the check and the call, the keyctl calls a NULL method:

  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

If it happens while the method is running, the method is left running in
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 ->asym_*() call, and check the
method again under it, right before the call.  While key->sem is held,
the GC cannot switch the key to key_type_dead, so unregister_key_type(),
and with it the module unload, cannot complete: the type, its module and
the payload stay in place until the method returns.  If the type is
already gone when key->sem is taken, the check finds key_type_dead and
the keyctl fails with -EOPNOTSUPP, as it does for any key type without
the method.

Otherwise, behaviour is unchanged:

 - The existing checks in keyctl_pkey_params_get(), keyctl_pkey_e_d_s()
   and keyctl_pkey_verify() are kept, so key types without the method
   still fail at the same point with the same error.  No call relies on
   them any more.

 - 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.

 - key->sem is only taken for reading, so pkey operations and KEYCTL_READ
   on the same key still run concurrently.  Writers such as KEYCTL_UPDATE,
   KEYCTL_REVOKE and KEYCTL_SETPERM now wait for a pkey operation in
   progress, as they already wait for ->read().  The in-tree ->asym_*()
   methods do not take key->sem themselves.

Nothing else in the kernel calls these three methods, so the four call
sites below are the full coverage:

 - keyctl_pkey_query(): ->asym_query() for KEYCTL_PKEY_QUERY
 - keyctl_pkey_params_get_2(): ->asym_query() to check the buffer sizes
   for KEYCTL_PKEY_ENCRYPT, _DECRYPT, _SIGN and _VERIFY
 - keyctl_pkey_e_d_s(): ->asym_eds_op() for KEYCTL_PKEY_ENCRYPT,
   _DECRYPT and _SIGN
 - keyctl_pkey_verify(): ->asym_verify_signature() for
   KEYCTL_PKEY_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").

LLM assistants, OpenAI Codex (gpt-6.1-sol) and Anthropic Claude
(claude-opus-5-5), found this while looking for other cases of the
KEYCTL_READ problem, and drafted this patch and this description.

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 v5:
- Keep the asym_query check in keyctl_pkey_params_get() (Jarkko), and
  for the same reason keep the asym_eds_op and asym_verify_signature
  checks that keyctl_pkey_e_d_s() and keyctl_pkey_verify() do before
  copying the input.  All four call sites now get the same change and
  nothing else moves.  This also restores the error for
  KEYCTL_PKEY_VERIFY with a bad input pointer on a key type without
  ->asym_verify_signature, such as turris-signing-key: v4 returned
  -EFAULT there instead of -EOPNOTSUPP.
- Rewrite the commit message around that change: what it does, why it
  is safe, and why the four call sites are all of them.
- Say in the changelog that LLM assistants found this bug and drafted
  the patch and its description.  They also wrote the tests below.

Tested on x86_64 under QEMU/KVM, with KASAN and lockdep, on 7.4-rc1
with and without this patch.  A test-only key type module (not for
merging) is unloaded while KEYCTL_PKEY_SIGN or KEYCTL_PKEY_VERIFY waits
in memdup_user() on a userfaultfd page, after the early check:

  without the patch: NULL call from keyctl_pkey_e_d_s() and
                     keyctl_pkey_verify()
  with the patch:    both return -EOPNOTSUPP

When the module's ->asym_eds_op() is sleeping as the unload starts, the
unpatched kernel faults on the freed module text; with the patch,
delete_module() returns only after the method has returned.
KEYCTL_PKEY_QUERY, _ENCRYPT, _DECRYPT, _SIGN and _VERIFY on an RSA-2048
asymmetric key give the same results with and without the patch.

In the two forced races lockdep also reports "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 that took its key
reference before the unload can hit this, e.g. keyctl_read_key().
Not tested on Turris hardware.

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 | 49 ++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 15b6bf6399..0ea6f2790e 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -129,7 +129,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_up_key_sem;
+	}
+
 	ret = params->key->type->asym_query(params, &info);
+
+out_up_key_sem:
+	up_read(&params->key->sem);
 	if (ret < 0)
 		return ret;
 
@@ -185,7 +194,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;
 
@@ -260,7 +278,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 +332,41 @@ 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;
+		goto out_free_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

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

[Jarkko Sakkinen]

On Sun, Nov 08, 2026 at 09:14:17PM +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 this happens
> between the check and the call, the keyctl calls a NULL method:
> 
>   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
> 
> If it happens while the method is running, the method is left running in
> 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 ->asym_*() call, and check the
> method again under it, right before the call.  While key->sem is held,
> the GC cannot switch the key to key_type_dead, so unregister_key_type(),
> and with it the module unload, cannot complete: the type, its module and
> the payload stay in place until the method returns.  If the type is
> already gone when key->sem is taken, the check finds key_type_dead and
> the keyctl fails with -EOPNOTSUPP, as it does for any key type without
> the method.
> 
> Otherwise, behaviour is unchanged:
> 
>  - The existing checks in keyctl_pkey_params_get(), keyctl_pkey_e_d_s()
>    and keyctl_pkey_verify() are kept, so key types without the method
>    still fail at the same point with the same error.  No call relies on
>    them any more.
> 
>  - 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.
> 
>  - key->sem is only taken for reading, so pkey operations and KEYCTL_READ
>    on the same key still run concurrently.  Writers such as KEYCTL_UPDATE,
>    KEYCTL_REVOKE and KEYCTL_SETPERM now wait for a pkey operation in
>    progress, as they already wait for ->read().  The in-tree ->asym_*()
>    methods do not take key->sem themselves.
> 
> Nothing else in the kernel calls these three methods, so the four call
> sites below are the full coverage:
> 
>  - keyctl_pkey_query(): ->asym_query() for KEYCTL_PKEY_QUERY
>  - keyctl_pkey_params_get_2(): ->asym_query() to check the buffer sizes
>    for KEYCTL_PKEY_ENCRYPT, _DECRYPT, _SIGN and _VERIFY
>  - keyctl_pkey_e_d_s(): ->asym_eds_op() for KEYCTL_PKEY_ENCRYPT,
>    _DECRYPT and _SIGN
>  - keyctl_pkey_verify(): ->asym_verify_signature() for
>    KEYCTL_PKEY_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").
> 
> LLM assistants, OpenAI Codex (gpt-6.1-sol) and Anthropic Claude
> (claude-opus-5-5), found this while looking for other cases of the
> KEYCTL_READ problem, and drafted this patch and this description.

This paragrah is no longer required and can be removed. I'll make only
sure that you know what you're doing. LLM is equal to a text editor as
far as I'm concerned, which also LLM is neither excuse for anything.

I think this is fine but there was a Sashiko issue left that
still needs to be fixed.


> 
> 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>


Br, Jarkko

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

[Yuansheng Yang]

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 this happens
between the check and the call, the keyctl calls a NULL method:

  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

If it happens while the method is running, the method is left running in
freed module text.  In-tree, turris-signing-key provides ->asym_query()
and ->asym_eds_op() and can be built as a module.

Every read of a ->asym_*() method pointer must therefore happen with
key->sem held for reading.  Take it around each method call and check the
method again under it, right before the call.  While key->sem is held,
the GC cannot switch the key to key_type_dead, so unregister_key_type(),
and with it the module unload, cannot complete: the type, its module and
the payload stay in place until the method returns.  If the type is
already gone when key->sem is taken, the check finds key_type_dead and
the keyctl fails with -EOPNOTSUPP, as it does for any key type without
the method.

The checks that keyctl_pkey_params_get(), keyctl_pkey_e_d_s() and
keyctl_pkey_verify() used to do before copying the input were themselves
unlocked reads of the method pointer.  Move them into the key->sem
critical section that keyctl_pkey_params_get_2() takes around
->asym_query(), after the buffer size validation, so that a key type
without the method still fails with -EOPNOTSUPP before memdup_user() and
with the same error precedence as before.  keyctl_pkey_params_get() no
longer looks at the type; its callers check ->asym_query() under key->sem
before they use it.

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.  key->sem
is only taken for reading, so pkey operations and KEYCTL_READ on the same
key still run concurrently.  Writers such as KEYCTL_UPDATE,
KEYCTL_REVOKE and KEYCTL_SETPERM now wait for a pkey operation in
progress, as they already wait for ->read().  The in-tree ->asym_*()
methods do not take key->sem themselves.

Nothing else in the kernel calls these three methods, so the four call
sites below are the full coverage:

 - keyctl_pkey_query(): ->asym_query() for KEYCTL_PKEY_QUERY
 - keyctl_pkey_params_get_2(): ->asym_query() and the op-specific method
   check for KEYCTL_PKEY_ENCRYPT, _DECRYPT, _SIGN and _VERIFY
 - keyctl_pkey_e_d_s(): ->asym_eds_op() for KEYCTL_PKEY_ENCRYPT,
   _DECRYPT and _SIGN
 - keyctl_pkey_verify(): ->asym_verify_signature() for
   KEYCTL_PKEY_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 v6:
- Address the Sashiko review of v5: keyctl_pkey_params_get(),
  keyctl_pkey_e_d_s() and keyctl_pkey_verify() still read a ->asym_*()
  method pointer without key->sem in their early checks.  Those checks
  are gone.  The op-specific check now happens in
  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.
  keyctl_pkey_params_get() no longer looks at the key type at all; both
  of its callers check ->asym_query() under key->sem before using it.
- 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.0 s vs 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.

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 | 103 ++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 27 deletions(-)

diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 15b6bf6399..5ac049b71a 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,45 +126,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 +214,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 +268,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 +294,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 +348,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

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

[Jarkko Sakkinen]

On Mon, Nov 09, 2026 at 01:52:04PM +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 this happens
> between the check and the call, the keyctl calls a NULL method:
> 
>   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
> 
> If it happens while the method is running, the method is left running in
> freed module text.  In-tree, turris-signing-key provides ->asym_query()
> and ->asym_eds_op() and can be built as a module.
> 
> Every read of a ->asym_*() method pointer must therefore happen with
> key->sem held for reading.  Take it around each method call and check the
> method again under it, right before the call.  While key->sem is held,
> the GC cannot switch the key to key_type_dead, so unregister_key_type(),
> and with it the module unload, cannot complete: the type, its module and
> the payload stay in place until the method returns.  If the type is
> already gone when key->sem is taken, the check finds key_type_dead and
> the keyctl fails with -EOPNOTSUPP, as it does for any key type without
> the method.
> 
> The checks that keyctl_pkey_params_get(), keyctl_pkey_e_d_s() and
> keyctl_pkey_verify() used to do before copying the input were themselves
> unlocked reads of the method pointer.  Move them into the key->sem
> critical section that keyctl_pkey_params_get_2() takes around
> ->asym_query(), after the buffer size validation, so that a key type
> without the method still fails with -EOPNOTSUPP before memdup_user() and
> with the same error precedence as before.  keyctl_pkey_params_get() no
> longer looks at the type; its callers check ->asym_query() under key->sem
> before they use it.
> 
> 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.  key->sem
> is only taken for reading, so pkey operations and KEYCTL_READ on the same
> key still run concurrently.  Writers such as KEYCTL_UPDATE,
> KEYCTL_REVOKE and KEYCTL_SETPERM now wait for a pkey operation in
> progress, as they already wait for ->read().  The in-tree ->asym_*()
> methods do not take key->sem themselves.
> 
> Nothing else in the kernel calls these three methods, so the four call
> sites below are the full coverage:
> 
>  - keyctl_pkey_query(): ->asym_query() for KEYCTL_PKEY_QUERY
>  - keyctl_pkey_params_get_2(): ->asym_query() and the op-specific method
>    check for KEYCTL_PKEY_ENCRYPT, _DECRYPT, _SIGN and _VERIFY
>  - keyctl_pkey_e_d_s(): ->asym_eds_op() for KEYCTL_PKEY_ENCRYPT,
>    _DECRYPT and _SIGN
>  - keyctl_pkey_verify(): ->asym_verify_signature() for
>    KEYCTL_PKEY_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 v6:
> - Address the Sashiko review of v5: keyctl_pkey_params_get(),
>   keyctl_pkey_e_d_s() and keyctl_pkey_verify() still read a ->asym_*()
>   method pointer without key->sem in their early checks.  Those checks
>   are gone.  The op-specific check now happens in
>   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.
>   keyctl_pkey_params_get() no longer looks at the key type at all; both
>   of its callers check ->asym_query() under key->sem before using it.
> - 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.0 s vs 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.
> 
> 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 | 103 ++++++++++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 27 deletions(-)
> 
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399..5ac049b71a 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,45 +126,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 +214,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 +268,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 +294,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 +348,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
> 

Thank you!

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

Br, Jarkko

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

[Jarkko Sakkinen]

On Mon, Nov 09, 2026 at 05:04:38PM +0200, Jarkko Sakkinen wrote:
> Thank you!
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

And the bug fix is not without value because now I can rebase my own
patch set on top of codebase that is not as broken as it was. Zero
disregard on that side.

Br, Jarkko

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

[Jarkko Sakkinen]

On Mon, Nov 09, 2026 at 01:52:04PM +0800, Yuansheng Yang wrote:
> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399..5ac049b71a 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;
>  }

I think I asked to remove this. I need to revoke my reviewed-by from
this version.

It's not relevant for the bug fix.

Also if possible try to make a bit more compact description of the
fix. I did not ask this for reviewed patch because did not see it
relevant enough but now that this needs a new round, you could
address this part too.

Br, Jarkko

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

[Yuansheng Yang]

On Tue, Nov 10, 2026 at 03:49:28AM +0200, Jarkko Sakkinen wrote:
> > -	if (!params->key->type->asym_query)
> > -		return -EOPNOTSUPP;
> > -
>
> I think I asked to remove this. I need to revoke my reviewed-by from
> this version.
>
> It's not relevant for the bug fix.

You did, in the v4 review, and v6 dropped the check again by mistake.
v7 restores it: keyctl_pkey_params_get() is identical to 7.4-rc1 again,
and the diff does not touch it at all.

The two early checks in keyctl_pkey_e_d_s() and keyctl_pkey_verify()
stay where v6 moved them, in the key->sem section of
keyctl_pkey_params_get_2() and after the buffer size validation, so the
error precedence is unchanged.  Only the check you named is restored.

Sashiko's remaining point on v5 was that this check reads key->type
without key->sem.  That is how 7.4-rc1 has it and you asked to keep the
hunk out of the patch, so v7 leaves it alone; the call sites recheck
the method under key->sem before they use it.

> Also if possible try to make a bit more compact description of the
> fix. I did not ask this for reviewed patch because did not see it
> relevant enough but now that this needs a new round, you could
> address this part too.

Done: the race diagram and the paragraph describing the early-check
move are gone; the problem, the action, why it is safe and the coverage
list stay.

v7 follows right after this mail.

Thanks,
Yuansheng Yang

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

[Yuansheng Yang]

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

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

[Jarkko Sakkinen]

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