[PATCH v2] keys: Protect the type name while describing a key

[PATCH v2] keys: Protect the type name while describing a key

[Cen Zhang]

keyctl_describe_key() must keep the key type's name alive until
kasprintf() has consumed it. The retained key reference keeps the key
and its description alive, but does not pin the module providing its
type. The name is passed to kasprintf() without holding key->sem.

kvasprintf() uses the saved name pointer in two formatting passes, with
a GFP_KERNEL allocation between them. With AF_RXRPC=m, a task that
passes View permission before type retirement can overlap a privileged
module unload in this order:

1. KEYCTL_DESCRIBE looks up the key, saves key->type->name in the
   kasprintf() arguments and completes the first formatting pass.
2. While the formatter is delayed before its second pass,
   af_rxrpc_exit() calls unregister_key_type(), which removes the type
   from the registry and waits for key_gc_keytype().
3. The collector takes key->sem for writing, retypes the retained key
   to key_type_dead and completes retirement. Unregistration and module
   exit return, allowing free_module() to release the type and name.
4. The formatter resumes its second pass and reads the saved name from
   released module storage, which can fault in string().

Hold key->sem for reading across kasprintf(). The collector then cannot
retype the key or complete retirement until both formatting passes have
finished. If retirement wins the lock first, formatting uses the core
key_type_dead name instead. Release the semaphore immediately after
kasprintf(), including on allocation failure, before copying to
userspace.

Oops report as below:

    BUG: unable to handle page fault for address: fffffbfff8054794
    #PF: supervisor read access in kernel mode
    #PF: error_code(0x0000) - not-present page
    RIP: 0010:string+0x23f/0x470
    Call Trace:
     vsnprintf+0x330/0x1110

Fixes: aa9d4437893f ("KEYS: Fix the size of the key description passed to/from userspace")
Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Changes in v2:
- Trim the Oops report to the fault location and vsnprintf frame.

Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0009-v3-43bbf21d1c452f23d829@gmail.com

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index d14ace88e529cb77de5386f1f3a99905d4b7688c..5a5efa26ed0ece3a5ab22e37ad7bda7a5a744b83 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -677,12 +677,14 @@ long keyctl_describe_key(key_serial_t keyid,
 
 	/* calculate how much information we're going to return */
 	ret = -ENOMEM;
+	down_read(&key->sem);
 	infobuf = kasprintf(GFP_KERNEL,
 			    "%s;%d;%d;%08x;",
 			    key->type->name,
 			    from_kuid_munged(current_user_ns(), key->uid),
 			    from_kgid_munged(current_user_ns(), key->gid),
 			    key->perm);
+	up_read(&key->sem);
 	if (!infobuf)
 		goto error2;
 	infolen = strlen(infobuf);

Re: [PATCH v2] keys: Protect the type name while describing a key

[Jarkko Sakkinen]

On Fri, Oct 09, 2026 at 01:11:03PM +0800, Cen Zhang wrote:
> keyctl_describe_key() must keep the key type's name alive until
> kasprintf() has consumed it. The retained key reference keeps the key
> and its description alive, but does not pin the module providing its
> type. The name is passed to kasprintf() without holding key->sem.
> 
> kvasprintf() uses the saved name pointer in two formatting passes, with
> a GFP_KERNEL allocation between them. With AF_RXRPC=m, a task that
> passes View permission before type retirement can overlap a privileged
> module unload in this order:
> 
> 1. KEYCTL_DESCRIBE looks up the key, saves key->type->name in the
>    kasprintf() arguments and completes the first formatting pass.
> 2. While the formatter is delayed before its second pass,
>    af_rxrpc_exit() calls unregister_key_type(), which removes the type
>    from the registry and waits for key_gc_keytype().
> 3. The collector takes key->sem for writing, retypes the retained key
>    to key_type_dead and completes retirement. Unregistration and module
>    exit return, allowing free_module() to release the type and name.
> 4. The formatter resumes its second pass and reads the saved name from
>    released module storage, which can fault in string().
> 
> Hold key->sem for reading across kasprintf(). The collector then cannot
> retype the key or complete retirement until both formatting passes have
> finished. If retirement wins the lock first, formatting uses the core
> key_type_dead name instead. Release the semaphore immediately after
> kasprintf(), including on allocation failure, before copying to
> userspace.
> 
> Oops report as below:
> 
>     BUG: unable to handle page fault for address: fffffbfff8054794
>     #PF: supervisor read access in kernel mode
>     #PF: error_code(0x0000) - not-present page
>     RIP: 0010:string+0x23f/0x470
>     Call Trace:
>      vsnprintf+0x330/0x1110
> 
> Fixes: aa9d4437893f ("KEYS: Fix the size of the key description passed to/from userspace")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Trim the Oops report to the fault location and vsnprintf frame.
> 
> Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0009-v3-43bbf21d1c452f23d829@gmail.com
> 
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529cb77de5386f1f3a99905d4b7688c..5a5efa26ed0ece3a5ab22e37ad7bda7a5a744b83 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -677,12 +677,14 @@ long keyctl_describe_key(key_serial_t keyid,
>  
>  	/* calculate how much information we're going to return */
>  	ret = -ENOMEM;
> +	down_read(&key->sem);
>  	infobuf = kasprintf(GFP_KERNEL,
>  			    "%s;%d;%d;%08x;",
>  			    key->type->name,
>  			    from_kuid_munged(current_user_ns(), key->uid),
>  			    from_kgid_munged(current_user_ns(), key->gid),
>  			    key->perm);
> +	up_read(&key->sem);
>  	if (!infobuf)
>  		goto error2;
>  	infolen = strlen(infobuf);

This would be even clear without scenario as it is quite obvious that
key->type can swap but I do appreciate having one of the scenarios
documented. Just saying that this is much better commit that would
have gone through :-)

Great work, thank you.

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

Br, Jarkko