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

[BeaCox <0xbeac@gmail.com>]

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