[PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters
Chengfeng Ye
nicoyip.dev at gmail.com
Wed Oct 14 17:52:04 UTC 2026
Dear Jarkko,
On Wed, Oct 14, 2026 at 7:34 PM Jarkko Sakkinen <jarkko at kernel.org> wrote:
>
> On Tue, Oct 13, 2026 at 05:03:57PM +0800, Chengfeng Ye wrote:
> > 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
>
> Last night I missed the text below.
>
> It is somewhat messy to the point that frankly I could not read it, or
> at least I did not understand it. Have you read it yourself?
>
> There is a whole eassay and I can only see that key->user can change
> unless key_user_lock is taken, and READ_ONCE() is required to pair with
> the writers.
That READ_ONCE() should be unnecessary, since key_user_lock now
serializes both readers and writers. I will double-check whether to
drop it.
> I don't understand what is going on down there, and what's your goal
> with it?
Yes, the messy commit message should be cleaned up before sending...
Sorry for that. Basically, it describes why the three `key->user` use
sites should be protected by the lock (because there's no
synchronization) and why the remaining access at other sites doesn't
need it (because other caller sites are synchronized). I will rephrase
the commit message to make it human-readable.
>
>
> >
> > 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 at vger.kernel.org
> > Assisted-by: GPT-6-Astra
> > Signed-off-by: Chengfeng Ye <nicoyip.dev at 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
> >
>
> Br, Jarkko
Best regards,
Chengfeng
More information about the Linux-security-module-archive
mailing list