Re: [PATCH] KEYS: Protect user key quota access during instantiation
flat view
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: 2026-10-12 13:00:16
Also in:
keyrings, lkml
On Mon, Oct 12, 2026 at 11:15:43AM +0800, Cen Zhang wrote:
The owner account must remain alive throughout payload quota reservation. The user key type calls generic_key_instantiate(), which dereferences key->user in key_payload_reserve(), without holding key->sem. The construction mutex excludes other instantiators but does not exclude keyctl_chown_key().
I'm not following the term "owner account". It is neither an
actor in the concurrency scenario. What do you exactly mean
by it?
A kernel caller can allocate an in-quota user key, retain its reference
and instantiate it with a nonempty payload after publishing its serial.
A concurrent task with Setattr permission and CAP_SYS_ADMIN can chown
the still-uninstantiated key through KEY_LOOKUP_PARTIAL. If the key
holds the last reference to its non-root account, instantiation has no
authorization key, and the new owner has sufficient quota, this ordering
is possible:
Instantiator Chown task
key_instantiate_and_link() keyctl_chown_key()
lock key_construction_mutex down_write(key->sem)
generic_key_instantiate()
key_payload_reserve()
load old = key->user
transfer quota to new owner
replace key->user
up_write(key->sem)
key_put(key)
key_user_put(old)
kfree(old)
read old->uid
Chown's final free falls between the old pointer load and the uid read.
The read can therefore access freed struct key_user storage despite the
caller retaining the key itself. Hold the read side of key->sem in a
local user instantiation callback across generic_key_instantiate().
Chown then cannot replace and release the account until the quota
accesses finish. If chown goes first, instantiation takes the semaphore
and uses the new account instead.
And account gets repeated here. I'm not sure hat accounts we are
talking about here.
I.e.
1. I get the scenario.
2. I read all of the text but I do not understand what I'm reading.
Keep locking out of the shared helper, which is also called by big_key_update() with key->sem write-locked.
On the other hand this could be a bit more verbose.
quoted hunk ↗ jump to hunk
KASAN report as below: BUG: KASAN: slab-use-after-free in key_payload_reserve+0x54d/0x560 Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner") Signed-off-by: Cen Zhang <redacted> Assisted-by: LLM ---diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c index 6f88b507f92791bd9569673c5d6534d76c58a978..b6e09921c11658c09eb7d7954b2abd5b2b7017c1 100644 --- a/security/keys/user_defined.c +++ b/security/keys/user_defined.c@@ -15,6 +15,7 @@ #include "internal.h" static int logon_vet_description(const char *desc); +static int user_instantiate(struct key *key, struct key_preparsed_payload *prep); /* * user defined keys take an arbitrary string as the description and an@@ -24,7 +25,7 @@ struct key_type key_type_user = { .name = "user", .preparse = user_preparse, .free_preparse = user_free_preparse, - .instantiate = generic_key_instantiate, + .instantiate = user_instantiate, .update = user_update, .revoke = user_revoke, .destroy = user_destroy,@@ -86,6 +87,19 @@ void user_free_preparse(struct key_preparsed_payload *prep) } EXPORT_SYMBOL_GPL(user_free_preparse); +/* + * Keep the owner account stable while reserving quota during instantiation. + */
Also here as a kernel developer I do not comprehend what I do when I
keep "an own account stable". I don't have kernel idioms that match.
quoted hunk ↗ jump to hunk
+static int user_instantiate(struct key *key, struct key_preparsed_payload *prep) +{ + int ret; + + down_read(&key->sem); + ret = generic_key_instantiate(key, prep); + up_read(&key->sem); + return ret; +} + static void user_free_payload_rcu(struct rcu_head *head) { struct user_key_payload *payload;
Not disagreeing with the fix but yeah would really like something else
than gibberish around the concurrency scenario.
Br, Jarkko