Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Jarkko Sakkinen]

On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> Protecting individual accesses to key->user does not make ownership
> transfers atomic with accounting updates. keyctl_chown_key() holds
> key->sem, but instantiation is serialized by key_construction_mutex and
> need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> an uninstantiated key.
> 
> The instantiated-key count can therefore be charged to the wrong owner:
> 
>   instantiate                       keyctl_chown_key()
>   lock key_user_lock
>   increment old->nikeys
>   unlock key_user_lock
>                                     observe KEY_IS_UNINSTANTIATED
>                                     skip the nikeys transfer
>                                     replace key->user
>   mark key instantiated
> 
> The key becomes instantiated under the new owner while the increment
> remains with the old owner. Negative instantiation has the same race.
> 
> Quota reservation can likewise run between charging the new owner and
> replacing key->user. It then adjusts the old owner's quota and changes
> key->quotalen while chown is transferring that quota burden.
> 
> Extend the key_user_lock critical section in keyctl_chown_key() across
> the quota and key-count transfers, state check, and owner replacement.
> Also extend the instantiation critical sections across the state update,
> so chown observes the count increment and instantiated state together.
> The existing per-user quota locks continue to serialize quota changes
> against other keys owned by the same user.
> 
> Keep allocations, notifications and reference release outside
> key_user_lock, and release it on the quota-overrun path.
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
> ---
> Changes in v3:
> - Split from v2 as patch 2/2; see the cover letter for the full split.
> - Rebase onto current mainline and retain explicit reader-side locking.
> 
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@xxxxxxxxx/
> 
>  security/keys/key.c    | 4 ++--
>  security/keys/keyctl.c | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index d0d583194b05..54c675b3b58d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
>  			/* mark the key as being instantiated */
>  			spin_lock(&key_user_lock);
>  			atomic_inc(&key->user->nikeys);
> -			spin_unlock(&key_user_lock);
>  			mark_key_instantiated(key, 0);
> +			spin_unlock(&key_user_lock);
>  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>  
>  			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
>  		/* mark the key as being negatively instantiated */
>  		spin_lock(&key_user_lock);
>  		atomic_inc(&key->user->nikeys);
> -		spin_unlock(&key_user_lock);
>  		mark_key_instantiated(key, -error);
> +		spin_unlock(&key_user_lock);
>  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
>  
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index c17924609317..83a9575b084e 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  		if (!newowner)
>  			goto error_put;
>  
> +		spin_lock(&key_user_lock);
> +
>  		/* transfer the quota burden to the new user */
>  		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>  			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> @@ -1036,11 +1038,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;
> +		spin_unlock(&key_user_lock);
>  	}
>  
>  	/* change the GID */
> @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  
>  quota_overrun:
>  	spin_unlock_irqrestore(&newowner->lock, flags);
> +	spin_unlock(&key_user_lock);
>  	zapowner = newowner;
>  	ret = -EDQUOT;
>  	goto error_put;
> -- 
> 2.43.0
> 


Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>

Br, Jarkko