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

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

[Cen Zhang]

keyctl_read_key() checks key->type->read without holding key->sem.
A reference keeps the key alive, but does not pin the module containing
its type. A surviving RxRPC key can therefore be read while AF_RXRPC
is being unloaded:

  KEYCTL_READ task                    Module unload and key GC
  ----------------                   ------------------------
  lookup_user_key()
  permission / possession checks
  load key->type
                                     af_rxrpc_exit()
                                       unregister_key_type()
                                         GC: down_write(key->sem)
                                         GC: retype to key_type_dead
                                         GC: up_write(key->sem)
                                       unregister_key_type() returns
                                     free_module()
  read saved type's read member

The final access can read freed module storage. The read invocation in
__keyctl_read_key() is already protected by key->sem, but the earlier
method check is outside that critical section.

Move the method check into __keyctl_read_key(), after key_validate() and
under its existing read hold on key->sem. The check and invocation then
use the same protected type for length queries and every read retry.
The GC cannot retype the key until the semaphore is released, and type
unregistration waits for that GC pass before the module can be freed.

Return -EOPNOTSUPP when validation succeeds and the type has no read
method. A temporary buffer may now be allocated before this result;
allocation and copying to userspace remain outside the semaphore.

Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

---
Changes in v2:
- Move the method check into __keyctl_read_key() under its existing read
  hold on key->sem and remove the outer check.
- Perform key validation before checking and invoking the read method.

Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0010-v5-c6b7ca2813c6913e500a@gmail.com

 security/keys/keyctl.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 5a5efa26ed0e..36ffba0ebba6 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -808,8 +808,12 @@ static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
 
 	down_read(&key->sem);
 	ret = key_validate(key);
-	if (ret == 0)
-		ret = key->type->read(key, buffer, buflen);
+	if (ret == 0) {
+		if (key->type->read)
+			ret = key->type->read(key, buffer, buflen);
+		else
+			ret = -EOPNOTSUPP;
+	}
 	up_read(&key->sem);
 	return ret;
 }
@@ -863,11 +867,6 @@ 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;
-		goto key_put_out;
-	}
-
 	if (!buffer || !buflen) {
 		/* Get the key length from the read method */
 		ret = __keyctl_read_key(key, NULL, 0);

base-commit: 89f5f41d296889f3b896c3ca67fcdf7d42bbdf10
-- 
2.43.0

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

[Jarkko Sakkinen]

On Mon, Oct 19, 2026 at 10:57:21PM +0800, Cen Zhang wrote:
> keyctl_read_key() checks key->type->read without holding key->sem.
> A reference keeps the key alive, but does not pin the module containing
> its type. A surviving RxRPC key can therefore be read while AF_RXRPC
> is being unloaded:
> 
>   KEYCTL_READ task                    Module unload and key GC
>   ----------------                   ------------------------
>   lookup_user_key()
>   permission / possession checks
>   load key->type
>                                      af_rxrpc_exit()
>                                        unregister_key_type()
>                                          GC: down_write(key->sem)
>                                          GC: retype to key_type_dead
>                                          GC: up_write(key->sem)
>                                        unregister_key_type() returns
>                                      free_module()
>   read saved type's read member
> 
> The final access can read freed module storage. The read invocation in
> __keyctl_read_key() is already protected by key->sem, but the earlier
> method check is outside that critical section.
> 
> Move the method check into __keyctl_read_key(), after key_validate() and
> under its existing read hold on key->sem. The check and invocation then
> use the same protected type for length queries and every read retry.
> The GC cannot retype the key until the semaphore is released, and type
> unregistration waits for that GC pass before the module can be freed.
> 
> Return -EOPNOTSUPP when validation succeeds and the type has no read
> method. A temporary buffer may now be allocated before this result;
> allocation and copying to userspace remain outside the semaphore.
> 
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

I literally described the fix to the reported bug instead of reviewing a
bug fix. That means by practical means that you only reported a bug.
This cannot happen again and if it does, next time I'll just do the
right fix myself and commit to the Git because that matches the reality
the best.


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

> 
> ---
> Changes in v2:
> - Move the method check into __keyctl_read_key() under its existing read
>   hold on key->sem and remove the outer check.
> - Perform key validation before checking and invoking the read method.
> 
> Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0010-v5-c6b7ca2813c6913e500a@gmail.com
> 
>  security/keys/keyctl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 5a5efa26ed0e..36ffba0ebba6 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -808,8 +808,12 @@ static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
>  
>  	down_read(&key->sem);
>  	ret = key_validate(key);
> -	if (ret == 0)
> -		ret = key->type->read(key, buffer, buflen);
> +	if (ret == 0) {
> +		if (key->type->read)
> +			ret = key->type->read(key, buffer, buflen);
> +		else
> +			ret = -EOPNOTSUPP;
> +	}
>  	up_read(&key->sem);
>  	return ret;
>  }
> @@ -863,11 +867,6 @@ 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;
> -		goto key_put_out;
> -	}
> -
>  	if (!buffer || !buflen) {
>  		/* Get the key length from the read method */
>  		ret = __keyctl_read_key(key, NULL, 0);
> 
> base-commit: 89f5f41d296889f3b896c3ca67fcdf7d42bbdf10
> -- 
> 2.43.0

Br, Jarkko

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

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> On Mon, Oct 19, 2026 at 10:57:21PM +0800, Cen Zhang wrote:
> > keyctl_read_key() checks key->type->read without holding key->sem.
> > A reference keeps the key alive, but does not pin the module containing
> > its type. A surviving RxRPC key can therefore be read while AF_RXRPC
> > is being unloaded:
> > 
> >   KEYCTL_READ task                    Module unload and key GC
> >   ----------------                   ------------------------
> >   lookup_user_key()
> >   permission / possession checks
> >   load key->type
> >                                      af_rxrpc_exit()
> >                                        unregister_key_type()
> >                                          GC: down_write(key->sem)
> >                                          GC: retype to key_type_dead
> >                                          GC: up_write(key->sem)
> >                                        unregister_key_type() returns
> >                                      free_module()
> >   read saved type's read member
> > 
> > The final access can read freed module storage. The read invocation in
> > __keyctl_read_key() is already protected by key->sem, but the earlier
> > method check is outside that critical section.
> > 
> > Move the method check into __keyctl_read_key(), after key_validate() and
> > under its existing read hold on key->sem. The check and invocation then
> > use the same protected type for length queries and every read retry.
> > The GC cannot retype the key until the semaphore is released, and type
> > unregistration waits for that GC pass before the module can be freed.
> > 
> > Return -EOPNOTSUPP when validation succeeds and the type has no read
> > method. A temporary buffer may now be allocated before this result;
> > allocation and copying to userspace remain outside the semaphore.
> > 
> > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Assisted-by: LLM
> > Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> 
> I literally described the fix to the reported bug instead of reviewing a
> bug fix. That means by practical means that you only reported a bug.
> This cannot happen again and if it does, next time I'll just do the
> right fix myself and commit to the Git because that matches the reality
> the best.
> 
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Reviewed-by: David Howells <dhowells@redhat.com>