* [PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters
2026-10-13 8:40 [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting Chengfeng Ye
@ 2026-10-13 9:03 ` Chengfeng Ye
2026-10-13 9:28 ` sashiko-bot
2026-10-13 9:03 ` [PATCH v4 2/2] keys: Serialize ownership transfers with key accounting Chengfeng Ye
2026-10-13 17:43 ` [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting Jarkko Sakkinen
2 siblings, 1 reply; 6+ messages in thread
From: Chengfeng Ye @ 2026-10-13 9:03 UTC (permalink / raw)
To: Jarkko Sakkinen, David Howells
Cc: Paul Moore, James Morris, Serge E . Hallyn, keyrings,
linux-security-module, linux-kernel, stable, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v4 2/2] keys: Serialize ownership transfers with key accounting
2026-10-13 8:40 [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting Chengfeng Ye
2026-10-13 9:03 ` [PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters Chengfeng Ye
@ 2026-10-13 9:03 ` Chengfeng Ye
2026-10-13 9:39 ` sashiko-bot
2026-10-13 17:43 ` [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting Jarkko Sakkinen
2 siblings, 1 reply; 6+ messages in thread
From: Chengfeng Ye @ 2026-10-13 9:03 UTC (permalink / raw)
To: Jarkko Sakkinen, David Howells
Cc: Paul Moore, James Morris, Serge E . Hallyn, keyrings,
linux-security-module, linux-kernel, stable, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting
2026-10-13 8:40 [PATCH v4 0/2] keys: Fix quota-owner reads and ownership accounting Chengfeng Ye
2026-10-13 9:03 ` [PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters Chengfeng Ye
2026-10-13 9:03 ` [PATCH v4 2/2] keys: Serialize ownership transfers with key accounting Chengfeng Ye
@ 2026-10-13 17:43 ` Jarkko Sakkinen
2 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2026-10-13 17:43 UTC (permalink / raw)
To: Chengfeng Ye
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
keyrings, linux-security-module, linux-kernel, stable
On Tue, Oct 13, 2026 at 04:40:54PM +0800, Chengfeng Ye wrote:
> 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
>
Hi overall this looks good (or better). I'm off work but I'll
look these up first thing in the morning and likely pick them
too.
Thanks for the patience and hope you understand that my previous
feedback was meant to be constructive :-)
Br, Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread