[PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting

[PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting

[Chengfeng Ye]

This revision keeps the v3 topic order: the lifetime/namespace-reader fix
is patch 1, and ownership accounting is patch 2. Each patch can be applied
independently to the base, and both orders produce the same final tree.

Patch 1 protects only the three quota-owner UID snapshots. It relies on
key_user_lock already held by lookup and final reference removal. Its
READ_ONCE() pointer loads pair with a WRITE_ONCE() replacement in chown.
The reference-release ordering and lock handoff keep a sampled owner alive;
this does not require patch 2's new accounting critical section. The commit
message gives the lifetime argument, including a final put by another task.

Patch 2 serializes chown with quota reservation and positive and negative
instantiation. Each critical section is complete within this commit.
Atomic counter operations alone do not serialize owner selection and
count/state publication with chown, and instantiation does not require the
target key's semaphore.

The messages explain why keeping a key alive does not pin its former
quota owner, and why key->sem does not exclude instantiation. They map
those two requirements to the three reader scopes and four accounting
scopes, and explain the existing protection at unchanged lifecycle sites.
All seven scopes in the combined v3 series are retained. Reservation and
instantiation locking move from v3 1/2 into the complete accounting fix
in v4 2/2; no lock scope is split between commits.

The procfs changes cover the originally reported proc_keys_start() fault.
The filters continue to use the quota-owner UID, since key_fsuid_changed()
can change a thread keyring's key->uid without moving its quota owner.

Changes in v4:
- Keep the v3 topic order: lifetime/namespace readers in patch 1,
  accounting in patch 2. Both fixes remain independently applicable.
- Put every accounting critical section wholly in patch 2.
- Make patch 1 independent using existing lifetime locking plus explicit
  pointer access marking.
- Explain the root cause and every critical section, plus the existing
  protection at unchanged sites. Keep allocation, notifications and
  reference release outside the new accounting scopes.
- Correct the reader patch's Fixes tags to the namespace-filter commits.
- Rebase onto for-next-keys at 22430ae5d90ab and request fresh review.

Validation:
- Built security/keys/ with patch 1 only, patch 2 only, and both patches.
  KEYS, PROC_FS, USER_NS, KEY_NOTIFICATIONS, BIG_KEYS, ENCRYPTED_KEYS and
  PROVE_LOCKING were enabled. No compiler warnings or errors.
- Both exported patches pass checkpatch --strict with zero diagnostics.
- Both patches apply directly to the base. Ordered git am in either order
  reproduces the expected intermediate and final trees.
- No new runtime, lockdep or Sashiko run. The KASAN excerpt is historical.

OpenAI Codex assisted with the review audit, series split, pointer access
marking, commit messages and build checks. The existing author sign-off
is retained from v3 for human review before submission.

Previous revisions:
v3: https://lore.kernel.org/r/20260927162528.943886-1-nicoyip.dev@gmail.com/
v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
v1: https://lore.kernel.org/r/20260823170448.3856516-1-nicoyip.dev@gmail.com/
Review:
https://lore.kernel.org/r/asL2UQjYCxt9SF9K@kernel.org/
https://lore.kernel.org/r/asMQMzqVkmYhwgMB@kernel.org/
https://lore.kernel.org/r/asNCIs3OshLwGWrB@kernel.org/

Chengfeng Ye (2):
  keys: Protect quota-owner reads in namespace filters
  keys: Serialize ownership transfers with key accounting

 security/keys/key.c     | 15 +++++++++++++--
 security/keys/keyctl.c  |  6 +++++-
 security/keys/keyring.c |  7 ++++++-
 security/keys/proc.c    | 15 +++++++++++++--
 4 files changed, 37 insertions(+), 6 deletions(-)


base-commit: 22430ae5d90ab288b0ee2ad99ae941f4a666b694
-- 
2.43.0

[PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters

[Chengfeng Ye]

keyctl_chown_key() replaces key->user under key->sem and drops the
reference to the previous owner after releasing the semaphore. Readers
which do not hold that semaphore can still be using the previous owner
when key_user_put() frees it.

For example, namespace filtering in /proc/keys can race with chown:

  /proc/keys reader                 keyctl_chown_key()
  user = key->user
                                    key->user = newowner
                                    key_user_put(old)
                                      kfree(old)
  read user->uid

An earlier instrumented run reported:

  BUG: KASAN: slab-use-after-free in proc_keys_start+0x353/0x440
  Read of size 4 at addr ffff8881128abbc0 by task poc/88
  Call Trace:
   proc_keys_start+0x353/0x440
   seq_read_iter+0x25d/0x1190
   proc_reg_read_iter+0x19e/0x260
  Allocated by task 86:
   key_user_lookup+0x1b4/0x540
   keyctl_chown_key+0x3cc/0xbf0
  Freed by task 87:
   kfree+0x149/0x330
   keyctl_chown_key+0x7b0/0xbf0

Take key_user_lock explicitly while /proc/keys and find_keyring_by_name()
copy the owner's UID. find_ge_key() is the initial/seek lookup reached
by the reported proc_keys_start() fault; key_serial_next() applies the
same UID filter to subsequent entries. find_keyring_by_name() reads the
same replaceable owner when looking up named keyrings.

key_serial_lock and keyring_name_lock protect these traversals but do
not exclude chown. Taking key->sem would sleep under those spinlocks.
The embedded key->user->lock cannot protect the dereference needed to
acquire it.

key_user_put() already holds key_user_lock for the final reference
decrement and tree removal. A reader selecting an owner before removal
blocks its final release until the UID copy finishes. After removal,
the lock and reference-count ordering make the preceding owner
replacement visible, including when another task drops the final
reference. Mark the pointer loads with READ_ONCE() and the chown store
with WRITE_ONCE(); the existing lifetime locking provides the protection.

Retain the quota-owner UID rather than substituting key->uid, since they
may differ for thread keyrings.

Other owner accesses already have lifetime protection. key_alloc() uses
a referenced owner before publishing the key. Final key_put() cannot
overlap chown, which holds a key reference. key_gc_unused_keys() handles
unreferenced keys: serial-tree removal excludes /proc/keys readers, and
keyring_destroy() removes named keyrings under keyring_name_lock before
the owner reference is dropped. /proc/key-users already holds
key_user_lock during traversal. Quota reservation and instantiation need
the separate accounting serialization.

Fixes: 454804ab0302 ("keys: make procfiles per-user-namespace")
Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
Cc: stable@vger.kernel.org
Assisted-by: GPT-6-Astra
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
Changes in v4:
- Move the quota-reservation and instantiation scopes from v3 1/2 to
  the complete accounting fix in 2/2; retain the combined lock coverage.
- Isolate the namespace-reader lifetime fix and make it independent of the
  accounting patch by relying on existing lookup/final-release locking.
- Pair READ_ONCE() pointer snapshots with WRITE_ONCE() in chown; explain
  lifetime ordering and preserve the quota-owner UID filtering semantics.
- Include analysis of of the other owner access sites in the changelog

v3: 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/keyctl.c  |  2 +-
 security/keys/keyring.c |  7 ++++++-
 security/keys/proc.c    | 15 +++++++++++++--
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index d14ace88e529c..c89a4d6efed40 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1039,3 +1039,3 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 		zapowner = key->user;
-		key->user = newowner;
+		WRITE_ONCE(key->user, newowner);
 		key->uid = uid;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 15bf4af8f2821..4a13749bc54cd 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1148,6 +1148,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 {
 	struct user_namespace *ns = current_user_ns();
 	struct key *keyring;
+	kuid_t uid;
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
@@ -1158,7 +1159,11 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 	 * grants Search permission and that hasn't been revoked
 	 */
 	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
-		if (!kuid_has_mapping(ns, keyring->user->uid))
+		spin_lock(&key_user_lock);
+		uid = READ_ONCE(keyring->user)->uid;
+		spin_unlock(&key_user_lock);
+
+		if (!kuid_has_mapping(ns, uid))
 			continue;
 
 		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 4f4e2c1824f18..aa42ea4becec7 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -68,7 +68,13 @@ static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
 	n = rb_next(n);
 	while (n) {
 		struct key *key = rb_entry(n, struct key, serial_node);
-		if (kuid_has_mapping(user_ns, key->user->uid))
+		kuid_t uid;
+
+		spin_lock(&key_user_lock);
+		uid = READ_ONCE(key->user)->uid;
+		spin_unlock(&key_user_lock);
+
+		if (kuid_has_mapping(user_ns, uid))
 			break;
 		n = rb_next(n);
 	}
@@ -80,6 +86,7 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
 	struct user_namespace *user_ns = seq_user_ns(p);
 	struct rb_node *n = key_serial_tree.rb_node;
 	struct key *minkey = NULL;
+	kuid_t uid;
 
 	while (n) {
 		struct key *key = rb_entry(n, struct key, serial_node);
@@ -100,7 +107,11 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
 		return NULL;
 
 	for (;;) {
-		if (kuid_has_mapping(user_ns, minkey->user->uid))
+		spin_lock(&key_user_lock);
+		uid = READ_ONCE(minkey->user)->uid;
+		spin_unlock(&key_user_lock);
+
+		if (kuid_has_mapping(user_ns, uid))
 			return minkey;
 		n = rb_next(&minkey->serial_node);
 		if (!n)
-- 
2.43.0

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

[Chengfeng Ye]

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

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@vger.kernel.org
Assisted-by: GPT-6-Astra
Signed-off-by: Chengfeng Ye <nicoyip.dev@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.
+ */
 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

Re: [PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters

[sashiko-bot]

> keyctl_chown_key() replaces key->user under key->sem and drops the
> reference to the previous owner after releasing the semaphore. Readers
> which do not hold that semaphore can still be using the previous owner
> when key_user_put() frees it.
> 
> For example, namespace filtering in /proc/keys can race with chown:
> 
>   /proc/keys reader                 keyctl_chown_key()
>   user = key->user
>                                     key->user = newowner
>                                     key_user_put(old)
>                                       kfree(old)
>   read user->uid
> 
> An earlier instrumented run reported:
> [ ... ]
> Fixes: 454804ab0302 ("keys: make procfiles per-user-namespace")
> Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
> Cc: stable@vger.kernel.org
> Assisted-by: GPT-6-Astra
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013084056.1070923-1-nicoyip.dev@gmail.com?part=1

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

[sashiko-bot]

> 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
> 
> [ ... ]
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@vger.kernel.org
> Assisted-by: GPT-6-Astra
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013084056.1070923-1-nicoyip.dev@gmail.com?part=2