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

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

[BeaCox]

keyctl_pkey_params_get() looks up the key, and keyctl_pkey_query(),
keyctl_pkey_e_d_s() and keyctl_pkey_verify() then dereference
key->type->asym_* outside key->sem.  A reference keeps the key alive, but
does not pin the module providing its type.

unregister_key_type() calls key_gc_keytype(), which retypes every
remaining key of the type to key_type_dead under key->sem for writing.
The pkey paths read key->type again after their checks, so a concurrent
module unload can turn the checked callback into the NULL key_type_dead
member and call through a NULL function pointer, e.g.:

  KEYCTL_PKEY_SIGN task              Module unload and key GC
  ------------------------           ------------------------
  keyctl_pkey_e_d_s()
    if (!key->type->asym_eds_op)     unregister_key_type()
                                       GC: down_write(&key->sem)
                                       GC: retype to key_type_dead
                                       GC: up_write(&key->sem)
                                     free_module()
    in = memdup_user(...)
    key->type->asym_eds_op(...)      -> NULL call

In-tree, turris-signing-key provides such methods; out-of-tree asymmetric
key types are affected as well.

Hold key->sem for reading from the lookup until the parameter block is
freed, so the type cannot be retired between the method check and the
matching call.

This is the same type-removal problem recently fixed for the read
(925c84e3cc21) and update (aef3a8a4f988) methods.

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: BeaCox <0xbeac@gmail.com>
---
 security/keys/keyctl_pkey.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 15b6bf6..b7e3648 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -17,7 +17,10 @@
 static void keyctl_pkey_params_free(struct kernel_pkey_params *params)
 {
 	kfree(params->info);
-	key_put(params->key);
+	if (params->key) {
+		up_read(&params->key->sem);
+		key_put(params->key);
+	}
 }
 
 enum {
@@ -100,6 +103,7 @@ static int keyctl_pkey_params_get(key_serial_t id,
 		return PTR_ERR(key_ref);
 	params->key = key_ref_to_ptr(key_ref);
 
+	down_read(&params->key->sem);
 	if (!params->key->type->asym_query)
 		return -EOPNOTSUPP;
 

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

[Jarkko Sakkinen]

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

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

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

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

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

Br, Jarkko

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

[BeaCox]

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: BeaCox <0xbeac@gmail.com>
---
Hi Jarkko,

Thanks for the review.  v2 keeps the semaphore ownership local: each keyctl
handler takes and releases key->sem itself, and the parameter block no
longer carries the lock out of the helper.

The type check and the matching ->asym_*() call are now in the same
read-side critical section.  keyctl_pkey_params_get_2() only covers its
->asym_query() call; the input buffers are copied before taking the lock
and the result is copied back after releasing it, so user memory is not
faulted in with key->sem held.

I also dropped the out-of-tree rationale.

I saw your "KEYS: Pin the backing kernel module for a key type" RFC.  If
that lands first this local fix should become unnecessary; I'm happy to
rebase or drop this patch, whichever you prefer.

v2 is below.  Tested with the same KASAN/QEMU setup: delete_module() now
succeeds while the caller is blocked in memdup_user(); the caller then
takes key->sem, sees key_type_dead, and returns -EOPNOTSUPP instead of
calling a NULL method.

Thanks,
Bea

Changes in v2:
- Keep key->sem ownership local to each keyctl handler; do not acquire it
  in keyctl_pkey_params_get() and release it in keyctl_pkey_params_free().
- Copy user buffers outside the key->sem critical section.
- Drop the out-of-tree key type rationale.

 security/keys/keyctl_pkey.c | 71 ++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 15b6bf6399..e553f6f311 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,14 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
 	if (ret < 0)
 		return ret;
 
+	down_read(&params->key->sem);
+	ret = -EOPNOTSUPP;
+	if (!params->key->type->asym_query)
+		goto out_unlock;
+
 	ret = params->key->type->asym_query(params, &info);
+out_unlock:
+	up_read(&params->key->sem);
 	if (ret < 0)
 		return ret;
 
@@ -185,15 +189,25 @@ long keyctl_pkey_query(key_serial_t id,
 	if (ret < 0)
 		goto error;
 
+	down_read(&params.key->sem);
+	ret = -EOPNOTSUPP;
+	if (!params.key->type->asym_query)
+		goto error_unlock;
+
 	ret = params.key->type->asym_query(&params, &res);
 	if (ret < 0)
-		goto error;
+		goto error_unlock;
+
+	up_read(&params.key->sem);
 
 	ret = -EFAULT;
 	if (copy_to_user(_res, &res, sizeof(res)) == 0 &&
 	    clear_user(_res->__spare, sizeof(_res->__spare)) == 0)
 		ret = 0;
+	goto error;
 
+error_unlock:
+	up_read(&params.key->sem);
 error:
 	keyctl_pkey_params_free(&params);
 	return ret;
@@ -230,9 +244,21 @@ long keyctl_pkey_e_d_s(int op,
 	if (ret < 0)
 		goto error_params;
 
+	in = memdup_user(_in, params.in_len);
+	if (IS_ERR(in)) {
+		ret = PTR_ERR(in);
+		goto error_params;
+	}
+
+	ret = -ENOMEM;
+	out = kmalloc(params.out_len, GFP_KERNEL);
+	if (!out)
+		goto error_in;
+
+	down_read(&params.key->sem);
 	ret = -EOPNOTSUPP;
 	if (!params.key->type->asym_eds_op)
-		goto error_params;
+		goto error_unlock;
 
 	switch (op) {
 	case KEYCTL_PKEY_ENCRYPT:
@@ -246,27 +272,18 @@ long keyctl_pkey_e_d_s(int op,
 		break;
 	default:
 		ret = -EOPNOTSUPP;
-		goto error_params;
+		goto error_unlock;
 	}
 
-	in = memdup_user(_in, params.in_len);
-	if (IS_ERR(in)) {
-		ret = PTR_ERR(in);
-		goto error_params;
-	}
-
-	ret = -ENOMEM;
-	out = kmalloc(params.out_len, GFP_KERNEL);
-	if (!out)
-		goto error_in;
-
 	ret = params.key->type->asym_eds_op(&params, in, out);
-	if (ret < 0)
-		goto error_out;
+	up_read(&params.key->sem);
 
-	if (copy_to_user(_out, out, ret) != 0)
+	if (ret >= 0 && copy_to_user(_out, out, ret) != 0)
 		ret = -EFAULT;
+	goto error_out;
 
+error_unlock:
+	up_read(&params.key->sem);
 error_out:
 	kfree(out);
 error_in:
@@ -307,10 +324,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);
@@ -323,9 +336,19 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
 		goto error_in;
 	}
 
+	down_read(&params.key->sem);
+	ret = -EOPNOTSUPP;
+	if (!params.key->type->asym_verify_signature)
+		goto error_unlock;
+
 	params.op = kernel_pkey_verify;
 	ret = params.key->type->asym_verify_signature(&params, in, in2);
+	up_read(&params.key->sem);
+	goto out;
 
+error_unlock:
+	up_read(&params.key->sem);
+out:
 	kfree(in2);
 error_in:
 	kfree(in);
-- 
2.43.0