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(¶ms->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(¶ms->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(¶ms.key->sem); + if (!params.key->type->asym_query) { + ret = -EOPNOTSUPP; + goto out_unlock; + } + ret = params.key->type->asym_query(¶ms, &res); + +out_unlock: + up_read(¶ms.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(¶ms.key->sem); + if (!params.key->type->asym_eds_op) { + ret = -EOPNOTSUPP; + goto out_unlock; + } + ret = params.key->type->asym_eds_op(¶ms, in, out); + +out_unlock: + up_read(¶ms.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(¶ms.key->sem); + if (!params.key->type->asym_verify_signature) { + ret = -EOPNOTSUPP; + goto out_unlock; + } + ret = params.key->type->asym_verify_signature(¶ms, in, in2); +out_unlock: + up_read(¶ms.key->sem); + kfree(in2); error_in: kfree(in); -- 2.43.0