Re: [PATCH] keys: Protect the read method check against type removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] keys: lockdep class use-after-free in keyctl_read_key() after type unload
--

commit 74db11a8500db2209c8e5cbb82e05cfc4e6396ee
Author: Cen Zhang <zzzccc427@gmail.com>

keys: Protect the read method check against type removal

This patch acquires key->sem for reading around the key->type->read check
in keyctl_read_key() so that a concurrent module unload and key garbage
collection pass cannot free the key type before the read pointer is
inspected.

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529c..43a5a3c4d1352 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -861,10 +861,13 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  
>  	/* the key is probably readable - now try to read it */
>  can_read_key:
> -	if (!key->type->read) {
> -		ret = -EOPNOTSUPP;
> +	down_read(&key->sem);
> +	ret = -EOPNOTSUPP;
> +	if (key->type->read)
> +		ret = 0;
> +	up_read(&key->sem);
> +	if (ret)
>  		goto key_put_out;
> -	}
>  
>  	if (!buffer || !buflen) {
>  		/* Get the key length from the read method */

[Severity: Medium]
With CONFIG_LOCKDEP enabled, can acquiring key->sem here trigger a
use-after-free on the lockdep class if the key's modular key_type has
already been unregistered and its module unloaded?

When the key is allocated in security/keys/key.c:key_alloc(), key->sem is
bound to the key type's lock_class in the module's data section:

    init_rwsem(&key->sem);
    lockdep_set_class(&key->sem, &type->lock_class);

If a reader task in keyctl_read_key() holds a reference to the key after
lookup_user_key() and is delayed before reaching can_read_key:, a
concurrent module unload can run unregister_key_type() ->
key_gc_keytype() -> key_garbage_collector() in security/keys/gc.c:

destroy_dead_key:
    spin_unlock(&key_serial_lock);
    kdebug("destroy key %d", key->serial);
    down_write(&key->sem);
    key->type = &key_type_dead;
    if (key_gc_dead_keytype->destroy)
        key_gc_dead_keytype->destroy(key);
    memset(&key->payload, KEY_DESTROY, sizeof(key->payload));
    up_write(&key->sem);

That down_write(&key->sem) populates key->sem.dep_map.class_cache[0] with
the module's lock_class, and key->type is switched to key_type_dead
without resetting the lockdep class on key->sem. Once
unregister_key_type() returns, free_module() -> lockdep_free_key_range()
-> zap_class() clears the class bit in lock_classes_in_use, sets
class->key = NULL, and queues the struct lock_class onto
free_lock_classes.

Previously, if retyping and module unload finished before can_read_key:,
keyctl_read_key() saw key_type_dead.read == NULL without taking key->sem.
With this change (or in __keyctl_read_key() if the module unloads after
can_read_key:), calling down_read(&key->sem) enters
kernel/locking/lockdep.c:__lock_acquire() with the dangling
key->sem.dep_map.class_cache[0] pointer:

    if (subclass < NR_LOCKDEP_CACHING_CLASSES)
        class = lock->class_cache[subclass];
    ...
    if (DEBUG_LOCKS_WARN_ON(!test_bit(class_idx, lock_classes_in_use)))
        return 0;

Could this hit DEBUG_LOCKS_WARN_ON(!test_bit(class_idx,
lock_classes_in_use)) or corrupt lockdep state if the lock_class entry
has been reallocated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-key-management-objects-candidate-0010-v5-c6b7ca2813c6913e500a@gmail.com?part=1