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

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

[Cen Zhang]

The key type must remain alive while its read method is checked.
keyctl_read_key() holds a reference to the key, but checks
key->type->read without holding key->sem. The reference keeps the key
alive, but does not pin the module containing its type.

With AF_RXRPC built as a module, a positive RxRPC key can pass
key_read_state() and the Read permission or possession checks while
another task unloads the module. Unload is possible once the module
refcount reaches zero, e.g. when no RxRPC socket remains open. The
reader can be delayed between loading key->type and reading its read
member, allowing this order:

  KEYCTL_READ task                  Module unload and key GC
  lookup_user_key()
  key_read_state()
  key_permission() / is_key_possessed()
  load key->type
                                    af_rxrpc_exit()
                                    unregister_key_type()
                                    key_gc_keytype() schedules the GC
                                      pass and waits for it
                                    GC takes key->sem for writing
                                    GC retypes key to key_type_dead
                                    GC releases key->sem
                                    unregister_key_type() returns
                                    free_module() frees type storage
  read saved type's read member

The last access reads freed module storage and can fault in
keyctl_read_key(). The later read invocation is already protected by
key->sem and key_validate() in __keyctl_read_key(), but neither covers
this earlier check.

Take key->sem for reading around the read-method check. The garbage
collector retypes the key under a write hold on key->sem, so it must
wait for the check to finish; key_gc_keytype() in turn waits for that
GC pass before unregister_key_type() returns, so the module text cannot
be freed until the check has completed. If retyping happens first, the
check sees key_type_dead, which has no read method. Release the
semaphore before returning -EOPNOTSUPP or proceeding to buffer
allocation, preserving the early unsupported-operation result and
keeping user copies outside the semaphore.

Oops report as below:

    BUG: unable to handle page fault for address: fffffbfff8048bc5
    #PF: supervisor read access in kernel mode
    #PF: error_code(0x0000) - not-present page
    RIP: 0010:keyctl_read_key+0x1aa/0x600
    Call Trace:
     __do_sys_keyctl+0x2a7/0x4f0

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Assisted-by: LLM

---
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 5a5efa26ed0e..756368422b7c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -863,10 +863,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 */

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

[sashiko-bot]

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

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

[Jarkko Sakkinen]

On Sat, Oct 17, 2026 at 03:30:56PM +0800, Cen Zhang wrote:
> The key type must remain alive while its read method is checked.
> keyctl_read_key() holds a reference to the key, but checks
> key->type->read without holding key->sem. The reference keeps the key
> alive, but does not pin the module containing its type.
> 
> With AF_RXRPC built as a module, a positive RxRPC key can pass
> key_read_state() and the Read permission or possession checks while
> another task unloads the module. Unload is possible once the module
> refcount reaches zero, e.g. when no RxRPC socket remains open. The
> reader can be delayed between loading key->type and reading its read
> member, allowing this order:
> 
>   KEYCTL_READ task                  Module unload and key GC
>   lookup_user_key()
>   key_read_state()
>   key_permission() / is_key_possessed()
>   load key->type
>                                     af_rxrpc_exit()
>                                     unregister_key_type()
>                                     key_gc_keytype() schedules the GC
>                                       pass and waits for it
>                                     GC takes key->sem for writing
>                                     GC retypes key to key_type_dead
>                                     GC releases key->sem
>                                     unregister_key_type() returns
>                                     free_module() frees type storage
>   read saved type's read member
> 
> The last access reads freed module storage and can fault in
> keyctl_read_key(). The later read invocation is already protected by
> key->sem and key_validate() in __keyctl_read_key(), but neither covers
> this earlier check.
> 
> Take key->sem for reading around the read-method check. The garbage
> collector retypes the key under a write hold on key->sem, so it must
> wait for the check to finish; key_gc_keytype() in turn waits for that
> GC pass before unregister_key_type() returns, so the module text cannot
> be freed until the check has completed. If retyping happens first, the
> check sees key_type_dead, which has no read method. Release the
> semaphore before returning -EOPNOTSUPP or proceeding to buffer
> allocation, preserving the early unsupported-operation result and
> keeping user copies outside the semaphore.
> 
> Oops report as below:
> 
>     BUG: unable to handle page fault for address: fffffbfff8048bc5
>     #PF: supervisor read access in kernel mode
>     #PF: error_code(0x0000) - not-present page
>     RIP: 0010:keyctl_read_key+0x1aa/0x600
>     Call Trace:
>      __do_sys_keyctl+0x2a7/0x4f0
> 
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> Assisted-by: LLM

Would be better to finish off the work unfinished and fix the patches
under circulation instead of spamming new ones while at it. I.e. fix
them first and don't starvate us with unfinished patches.

If this was merge ready this would be acceptable, but in this even the
tags above are in the wrong order. This is not a lottery machine where
you dump whatever LLM gives you without your attention, so please don't
do that.

> 
> ---
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 5a5efa26ed0e..756368422b7c 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -863,10 +863,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 */

Why did you chose this over embedding the check inside __keyctl_read_key()?

I'm assuming you have considered both options.

Br, Jarkko

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

[Jarkko Sakkinen]

On Sun, Oct 18, 2026 at 05:00:14AM +0300, Jarkko Sakkinen wrote:
> On Sat, Oct 17, 2026 at 03:30:56PM +0800, Cen Zhang wrote:
> > The key type must remain alive while its read method is checked.
> > keyctl_read_key() holds a reference to the key, but checks
> > key->type->read without holding key->sem. The reference keeps the key
> > alive, but does not pin the module containing its type.
> > 
> > With AF_RXRPC built as a module, a positive RxRPC key can pass
> > key_read_state() and the Read permission or possession checks while
> > another task unloads the module. Unload is possible once the module
> > refcount reaches zero, e.g. when no RxRPC socket remains open. The
> > reader can be delayed between loading key->type and reading its read
> > member, allowing this order:
> > 
> >   KEYCTL_READ task                  Module unload and key GC
> >   lookup_user_key()
> >   key_read_state()
> >   key_permission() / is_key_possessed()
> >   load key->type
> >                                     af_rxrpc_exit()
> >                                     unregister_key_type()
> >                                     key_gc_keytype() schedules the GC
> >                                       pass and waits for it
> >                                     GC takes key->sem for writing
> >                                     GC retypes key to key_type_dead
> >                                     GC releases key->sem
> >                                     unregister_key_type() returns
> >                                     free_module() frees type storage
> >   read saved type's read member
> > 
> > The last access reads freed module storage and can fault in
> > keyctl_read_key(). The later read invocation is already protected by
> > key->sem and key_validate() in __keyctl_read_key(), but neither covers
> > this earlier check.
> > 
> > Take key->sem for reading around the read-method check. The garbage
> > collector retypes the key under a write hold on key->sem, so it must
> > wait for the check to finish; key_gc_keytype() in turn waits for that
> > GC pass before unregister_key_type() returns, so the module text cannot
> > be freed until the check has completed. If retyping happens first, the
> > check sees key_type_dead, which has no read method. Release the
> > semaphore before returning -EOPNOTSUPP or proceeding to buffer
> > allocation, preserving the early unsupported-operation result and
> > keeping user copies outside the semaphore.
> > 
> > Oops report as below:
> > 
> >     BUG: unable to handle page fault for address: fffffbfff8048bc5
> >     #PF: supervisor read access in kernel mode
> >     #PF: error_code(0x0000) - not-present page
> >     RIP: 0010:keyctl_read_key+0x1aa/0x600
> >     Call Trace:
> >      __do_sys_keyctl+0x2a7/0x4f0
> > 
> > Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> > Assisted-by: LLM
> 
> Would be better to finish off the work unfinished and fix the patches
> under circulation instead of spamming new ones while at it. I.e. fix
> them first and don't starvate us with unfinished patches.
> 
> If this was merge ready this would be acceptable, but in this even the
> tags above are in the wrong order. This is not a lottery machine where
> you dump whatever LLM gives you without your attention, so please don't
> do that.
> 
> > 
> > ---
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index 5a5efa26ed0e..756368422b7c 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -863,10 +863,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 */
> 
> Why did you chose this over embedding the check inside __keyctl_read_key()?
> 
> I'm assuming you have considered both options.

I.e. to reformalize: how does that change guarantee key->type->read
validity for the __keyctl_read_key() that follow?

My first idea would be put the check inside __keyctl_read_key() 
while key->sem is taken.

Please explain because I'm not fully following with that change.
I could get something wrong but it is against the assumed most 
common sense fix imaginable.

Br, Jarkko