[PATCH v4 2/2] keys: Serialize ownership transfers with key accounting
Jarkko Sakkinen
jarkko at kernel.org
Wed Oct 14 11:36:49 UTC 2026
On Tue, Oct 13, 2026 at 05:03:58PM +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()
> increment old->nikeys
> observe KEY_IS_UNINSTANTIATED
> skip the nikeys transfer
> replace key->user
> mark key instantiated
Same here there's another below.
>
> 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.
> key_payload_reserve() can run during instantiation without key->sem,
> so it needs the same exclusion against chown.
>
> Hold key_user_lock in keyctl_chown_key() across the quota and key-count
> transfers, state check, and owner replacement; locking only the pointer
> store leaves the preceding accounting exposed. In
> __key_instantiate_and_link() and key_reject_and_link(), hold it across
> the instantiated-key count increment and state update, so chown observes
> both together. Atomic counter operations alone do not stabilize the
> owner or couple the increment with state publication.
>
> Hold the same lock in key_payload_reserve() from owner selection through
> updating qnbytes and key->quotalen. The per-user quota lock cannot
> stabilize key->user during a transfer. Retain the existing per-user locks
> 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.
>
> Normal updates through key_update() and __key_update() already hold
> key->sem and exclude chown. key_alloc() charges a referenced owner before
> publishing the key. Final key_put() and key_gc_unused_keys() cannot race
> with chown, which requires a key reference. Their existing per-user locks
> and atomic counters still protect totals shared with other keys.
> key_user_lookup()/key_user_put() already protect owner creation and final
> removal with key_user_lock. These sites need no new ownership lock.
>
> This accounting race was identified by source inspection.
>
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable at vger.kernel.org
> Assisted-by: GPT-6-Astra
> Signed-off-by: Chengfeng Ye <nicoyip.dev at gmail.com>
> ---
> Changes in v4:
> - Include the reservation and instantiation scopes formerly in v3 1/2;
> retain the same combined lock coverage while making this fix complete.
> - Keep accounting serialization as patch 2/2, matching v3. Include the
> full lock/count/state/unlock scope at both instantiation sites.
> - Include quota reservation and the entire chown accounting transaction;
> this patch can be applied without the namespace-reader fix.
>
> v3: https://lore.kernel.org/r/20260927162528.943886-3-nicoyip.dev@gmail.com/
> v3 1/2: https://lore.kernel.org/r/20260927162528.943886-2-nicoyip.dev@gmail.com/
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
>
> security/keys/key.c | 15 +++++++++++++--
> security/keys/keyctl.c | 4 ++++
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/key.c b/security/keys/key.c
> index a438c4508595d..61dd991cee543 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -21,6 +21,9 @@ struct rb_root key_serial_tree; /* tree of keys indexed by serial */
> DEFINE_SPINLOCK(key_serial_lock);
>
> struct rb_root key_user_tree; /* tree of quota records indexed by UID */
> +/*
> + * Protects key_user_tree and serializes ownership changes with key accounting.
> + */
Umh, why do you see this addition necessary in order to address bug? You
have to remove it because it is a spurious change.
> DEFINE_SPINLOCK(key_user_lock);
>
> unsigned int key_quota_root_maxkeys = 1000000; /* root's key count quota */
> @@ -380,9 +383,12 @@ int key_payload_reserve(struct key *key, size_t datalen)
>
> /* contemplate the quota adjustment */
> if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> - unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> - key_quota_root_maxbytes : key_quota_maxbytes;
> unsigned long flags;
> + unsigned int maxbytes;
> +
> + spin_lock(&key_user_lock);
> + maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> + key_quota_root_maxbytes : key_quota_maxbytes;
>
> spin_lock_irqsave(&key->user->lock, flags);
>
> @@ -396,6 +402,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
> key->quotalen += delta;
> }
> spin_unlock_irqrestore(&key->user->lock, flags);
> + spin_unlock(&key_user_lock);
> }
>
> /* change the recorded data length if that didn't generate an error */
> @@ -447,8 +454,10 @@ static int __key_instantiate_and_link(struct key *key,
>
> if (ret == 0) {
> /* mark the key as being instantiated */
> + spin_lock(&key_user_lock);
> 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))
> @@ -604,8 +613,10 @@ int key_reject_and_link(struct key *key,
> /* can't instantiate twice */
> if (key->state == KEY_IS_UNINSTANTIATED) {
> /* mark the key as being negatively instantiated */
> + spin_lock(&key_user_lock);
> 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);
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index c89a4d6efed40..e74fa0076ee0b 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1006,2 +1006,4 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>
> + spin_lock(&key_user_lock);
> +
> /* transfer the quota burden to the new user */
> @@ -1041,2 +1043,3 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> key->uid = uid;
> + spin_unlock(&key_user_lock);
> }
> @@ -1060,2 +1063,3 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> spin_unlock_irqrestore(&newowner->lock, flags);
> + spin_unlock(&key_user_lock);
> zapowner = newowner;
> --
> 2.43.0
>
Br, Jarkko
More information about the Linux-security-module-archive
mailing list