Re: [PATCH] KEYS: Protect user key quota access during instantiation

[Jarkko Sakkinen <jarkko@kernel.org>]

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.

> 
> 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 <zzzccc427@gmail.com>
> 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.

> +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