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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Oct 05, 2026 at 05:17:42AM +0300, Jarkko Sakkinen wrote:
> 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@vger.kernel.org
> > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > ---
> > 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@gmail.com/
> > 
> >  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
> > 
> 
> I double-checked this patch too given the concerns on 1/2 but nope, this
> does not raise similar concerns as every critical section is strictly
> related to ownership change.
> 
> E.g., I can be sure that the granularity is where it should be and locks
> are actually needed in the first place.
> 
> Thus, I'm still including this patch to my next PR, and drop the first
> one.

By dropping 1/2 I found out that 2/2 key.c becomes:

diff --git a/security/keys/key.c b/security/keys/key.c
index a438c4508595..de63120c51ad 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -449,6 +449,7 @@ static int __key_instantiate_and_link(struct key *key,
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
 			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))
@@ -606,6 +607,7 @@ int key_reject_and_link(struct key *key,
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
 		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);

This type of interleaving should never happen in a patch series.

Please don't rush your changes like this in future.

Br, Jarkko