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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KEYS: unlocked key->type dereferences in keyctl_pkey method
  checks
--

commit 0ed416869e71a7b6cc7a35c0985435f3666b373b
Author: Yuansheng Yang <0xbeac@gmail.com>

KEYS: Protect pkey method checks against type removal

This commit takes key->sem for reading around the four asymmetric key
method calls in security/keys/keyctl_pkey.c and re-checks the method
pointer under the lock to prevent races with key type unregistration.

> diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
> index 15b6bf6399b59..0ea6f2790e9de 100644
> --- a/security/keys/keyctl_pkey.c
> +++ b/security/keys/keyctl_pkey.c

[ ... ]

> @@ -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;

[Severity: Medium]
Can the remaining unlocked checks of key->type still fault on freed module
memory when a key type module is unloaded?

Checking key->type->asym_verify_signature here (and the corresponding
unlocked checks of key->type->asym_eds_op in keyctl_pkey_e_d_s() and
key->type->asym_query in keyctl_pkey_params_get()) reads key->type and
then dereferences the struct key_type in module memory without holding
key->sem:

CPU0                                  CPU1
keyctl_pkey_verify()
  reg = params.key->type
                                      unregister_key_type()
                                        ...
                                        down_write(&key->sem)
                                        key->type = &key_type_dead
                                        up_write(&key->sem)
                                      free_module()
  if (!reg->asym_verify_signature)  <-- use-after-free

security/keys/keyctl_pkey.c:keyctl_pkey_params_get() {
	...
	key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
	if (IS_ERR(key_ref))
		return PTR_ERR(key_ref);
	params->key = key_ref_to_ptr(key_ref);

	if (!params->key->type->asym_query)
		return -EOPNOTSUPP;

	return 0;
}

Both callers of keyctl_pkey_params_get() immediately take key->sem and
re-check asym_query before doing anything else, so the unlocked check in
keyctl_pkey_params_get() can be dropped, and the early asym_eds_op and
asym_verify_signature checks can be performed under key->sem inside
keyctl_pkey_params_get_2() while still returning -EOPNOTSUPP before
memdup_user().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108131417.77287-1-0xbeac@gmail.com?part=1