[PATCH v2] KEYS: Protect pkey method checks against type removal
From: BeaCox
Date: Tue Nov 03 2026 - 21:15:52 EST
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@xxxxxxxxxxxxxxx
Signed-off-by: BeaCox <0xbeac@xxxxxxxxx>
---
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(¶ms->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(¶ms->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(¶ms.key->sem);
+ ret = -EOPNOTSUPP;
+ if (!params.key->type->asym_query)
+ goto error_unlock;
+
ret = params.key->type->asym_query(¶ms, &res);
if (ret < 0)
- goto error;
+ goto error_unlock;
+
+ up_read(¶ms.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(¶ms.key->sem);
error:
keyctl_pkey_params_free(¶ms);
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(¶ms.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(¶ms, in, out);
- if (ret < 0)
- goto error_out;
+ up_read(¶ms.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(¶ms.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(¶ms.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(¶ms, in, in2);
+ up_read(¶ms.key->sem);
+ goto out;
+error_unlock:
+ up_read(¶ms.key->sem);
+out:
kfree(in2);
error_in:
kfree(in);
--
2.43.0