[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@xxxxxxxxxxxxxxx
Signed-off-by: Yuansheng Yang <0xbeac@xxxxxxxxx>
---
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@xxxxxxxxx/
v3: https://lore.kernel.org/all/9d47e202bd11857b3555e6b95694b3c3a4f2460b.1793852399.git.0xbeac@xxxxxxxxx/
v2: https://lore.kernel.org/all/20261104021414.50696-1-0xbeac@xxxxxxxxx/
v1: https://lore.kernel.org/all/20261103032940.51826-1-0xbeac@xxxxxxxxx/

 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