Re: [PATCH] keys: Fix key_user use-after-free during ownership changes

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sun, Aug 23, 2026 at 11:28:58PM +0800, Chengfeng Ye wrote:
> keyctl_chown_key() replaces key->user while holding key->sem and drops
> the old key_user reference after releasing the semaphore. The /proc/keys
> iterators and find_keyring_by_name() instead dereference key->user while
> holding unrelated locks.
> 
> This allows the following interleaving:
> 
>   CPU 0 (/proc/keys)            CPU 1 (KEYCTL_CHOWN)
>   load old key->user
>                                 replace key->user
>                                 key_user_put(old)
>                                   kfree(old)
>   read old->uid
> 
> Serialize the namespace-mapping reads and the pointer replacement with
> key_user_lock. This lock already protects final key_user removal, so the
> old object cannot be freed while its uid is being read. Keep reading the
> quota-owner UID rather than key->uid because those values can legitimately
> differ for thread keyrings.
> 
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>  security/keys/internal.h | 10 ++++++++++
>  security/keys/keyctl.c   |  2 ++
>  security/keys/keyring.c  |  2 +-
>  security/keys/proc.c     |  4 ++--
>  4 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index b7b622bc36a1..741d547ba5c4 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -70,6 +70,16 @@ extern struct key_user	root_key_user;
>  extern struct key_user *key_user_lookup(kuid_t uid);
>  extern void key_user_put(struct key_user *user);
>  
> +static inline kuid_t key_user_uid(const struct key *key)
> +{
> +	kuid_t uid;
> +
> +	spin_lock(&key_user_lock);
> +	uid = key->user->uid;
> +	spin_unlock(&key_user_lock);
> +	return uid;
> +}
> +
>  /*
>   * Key quota limits.
>   * - root has its own separate limits to everyone else
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529..c17924609317 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1036,8 +1036,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  			atomic_inc(&newowner->nikeys);
>  		}
>  
> +		spin_lock(&key_user_lock);
>  		zapowner = key->user;
>  		key->user = newowner;
> +		spin_unlock(&key_user_lock);
>  		key->uid = uid;
>  	}
>  
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 15bf4af8f282..49f4be934525 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1158,7 +1158,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
>  	 * grants Search permission and that hasn't been revoked
>  	 */
>  	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
> -		if (!kuid_has_mapping(ns, keyring->user->uid))
> +		if (!kuid_has_mapping(ns, key_user_uid(keyring)))
>  			continue;
>  
>  		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index 4f4e2c1824f1..8d6d26652aab 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -68,7 +68,7 @@ static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
>  	n = rb_next(n);
>  	while (n) {
>  		struct key *key = rb_entry(n, struct key, serial_node);
> -		if (kuid_has_mapping(user_ns, key->user->uid))
> +		if (kuid_has_mapping(user_ns, key_user_uid(key)))
>  			break;
>  		n = rb_next(n);
>  	}
> @@ -100,7 +100,7 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
>  		return NULL;
>  
>  	for (;;) {
> -		if (kuid_has_mapping(user_ns, minkey->user->uid))
> +		if (kuid_has_mapping(user_ns, key_user_uid(minkey)))
>  			return minkey;
>  		n = rb_next(&minkey->serial_node);
>  		if (!n)
> -- 
> 2.43.0
> 

I hope you don't mind that I sanity check a bit:

 git grep "user->uid" security/keys
security/keys/internal.h:       uid = key->user->uid;
security/keys/key.c:            if (uid_lt(uid, user->uid))
security/keys/key.c:            else if (uid_gt(uid, user->uid))
security/keys/key.c:            unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
security/keys/proc.c:           if (kuid_has_mapping(user_ns, user->uid))
security/keys/proc.c:   unsigned maxkeys = uid_eq(user->uid, GLOBAL_ROOT_UID) ?
security/keys/proc.c:   unsigned maxbytes = uid_eq(user->uid, GLOBAL_ROOT_UID) ?
security/keys/proc.c:              from_kuid_munged(seq_user_ns(m), user->uid),
security/keys/process_keys.c:   uid_t uid = from_kuid(user_ns, cred->user->uid);
security/keys/process_keys.c:           uid_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
security/keys/process_keys.c:           session_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
security/keys/process_keys.c:                                               cred->user->uid));

Have you audited these and reflected against your current patch?

BR, Jarkko