[PATCH v3 2/2] keys: Serialize ownership transfers with key accounting
Chengfeng Ye
nicoyip.dev at gmail.com
Mon Oct 5 05:53:29 UTC 2026
On Mon, Oct 5, 2026 at 10:49 AM Jarkko Sakkinen <jarkko at 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 at vger.kernel.org
> > > Signed-off-by: Chengfeng Ye <nicoyip.dev at 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.
Best regards,
Chengfeng
More information about the Linux-security-module-archive
mailing list