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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Oct 05, 2026 at 01:53:29PM +0800, Chengfeng Ye wrote:
> On Mon, Oct 5, 2026 at 10:49 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > 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
> 
> Sorry for the mistake, I should have carefully verified that the tree
> remained valid at each step of the series.
> 
> I will rework the locking on 1/2 to inspect if there might be any
> unnecessary coverage, and verify each intermediate commit
> independently before sending a revised version for the patch series.

No problem. I just wanted to underline this :-) And I will gladly take
part of the blame given that I also rushed and this should have been
catched and fixed during the review cycle.

So let's just address this, phase down a bit so that at least correct
stuff gets out (and as many review cycles that requires).

The basis of the fix is not rotten. It does address the correct
root cause and is going into right direction. It's just not taken
to the end.

> 
> Best regards,
> Chengfeng

Br, Jarkko