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

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

[Cen Zhang]

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().

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.

Keep locking out of the shared helper, which is also called by
big_key_update() with key->sem write-locked.

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.
+ */
+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;

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

[Jarkko Sakkinen]

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

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

[Jarkko Sakkinen]

On Mon, Oct 12, 2026 at 04:00:16PM +0300, Jarkko Sakkinen wrote:
> 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.

I appreciate the fix but if I'm completely hones the commit message
looks like direct LLM output, which is not acceptable.

It needs to be proof-readed and edited. For this reason, I want a better
iteration before I apply this.

Br, Jarkko

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

[Cen Zhang]

Hi Jarkko,

Thanks very much for the detailed feedback and for your patience with this :).

Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月12日周一 21:03写道：
>
>
> I appreciate the fix but if I'm completely hones the commit message
> looks like direct LLM output, which is not acceptable.
>
> It needs to be proof-readed and edited. For this reason, I want a better
> iteration before I apply this.
>
> Br, Jarkko

Thanks very much for the detailed feedback and for your patience with this.

Several people have mentioned that my commit messages look like they were
generated by an LLM — and to be honest, I did write them with LLM assistance.
However, they often gave me the feeling of being questioned rather
than explained.
I spend most of my time looking for bugs, following my interest, so
I’m more familiar
with bug patterns than with the deeper details of the subsystem. That
means I can
make mistakes. I’m willing to listen to feedback and keep revising the
patch until it’s in
good shape, but the pressure sometimes makes me afraid to continue the
conversation,
worried about making more mistakes. Your patient explanation this time
really means a
lot to me.

Of course, I also fully understand that busy developers feel
uncomfortable when seeing
unclear or poorly written messages. I just wanted to express my thanks
for taking the time
to gently and patiently explain things clearly.

I’ll carefully rework the commit message following your suggestions to
make the concurrency
scenario and the reasoning clearer, and send a new patch once it’s
ready. I’d appreciate it if
you could take another look then.

Best regards,
Cen Zhang

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

[Cen Zhang]

Hi Jarkko,

Sorry for the previous email — the line wrapping got a bit messy on my
side. Hope it was still readable :).

Best regards,
Cen

Cen Zhang <zzzccc427@gmail.com> 于2026年10月12日周一 21:30写道：
>
> Hi Jarkko,
>
> Thanks very much for the detailed feedback and for your patience with this :).
>
> Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月12日周一 21:03写道：
> >
> >
> > I appreciate the fix but if I'm completely hones the commit message
> > looks like direct LLM output, which is not acceptable.
> >
> > It needs to be proof-readed and edited. For this reason, I want a better
> > iteration before I apply this.
> >
> > Br, Jarkko
>
> Thanks very much for the detailed feedback and for your patience with this.
>
> Several people have mentioned that my commit messages look like they were
> generated by an LLM — and to be honest, I did write them with LLM assistance.
> However, they often gave me the feeling of being questioned rather
> than explained.
> I spend most of my time looking for bugs, following my interest, so
> I’m more familiar
> with bug patterns than with the deeper details of the subsystem. That
> means I can
> make mistakes. I’m willing to listen to feedback and keep revising the
> patch until it’s in
> good shape, but the pressure sometimes makes me afraid to continue the
> conversation,
> worried about making more mistakes. Your patient explanation this time
> really means a
> lot to me.
>
> Of course, I also fully understand that busy developers feel
> uncomfortable when seeing
> unclear or poorly written messages. I just wanted to express my thanks
> for taking the time
> to gently and patiently explain things clearly.
>
> I’ll carefully rework the commit message following your suggestions to
> make the concurrency
> scenario and the reasoning clearer, and send a new patch once it’s
> ready. I’d appreciate it if
> you could take another look then.
>
> Best regards,
> Cen Zhang

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

[Jarkko Sakkinen]

On Mon, Oct 12, 2026 at 09:33:06PM +0800, Cen Zhang wrote:
> Hi Jarkko,
> 
> Sorry for the previous email — the line wrapping got a bit messy on my
> side. Hope it was still readable :).

All cool.

> 
> Best regards,
> Cen

Br, Jarkko

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

[Jarkko Sakkinen]

On Mon, Oct 12, 2026 at 09:30:43PM +0800, Cen Zhang wrote:
> Hi Jarkko,
> 
> Thanks very much for the detailed feedback and for your patience with this :).
> 
> Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月12日周一 21:03写道：
> >
> >
> > I appreciate the fix but if I'm completely hones the commit message
> > looks like direct LLM output, which is not acceptable.
> >
> > It needs to be proof-readed and edited. For this reason, I want a better
> > iteration before I apply this.
> >
> > Br, Jarkko
> 
> Thanks very much for the detailed feedback and for your patience with this.
> 
> Several people have mentioned that my commit messages look like they were
> generated by an LLM — and to be honest, I did write them with LLM assistance.
> However, they often gave me the feeling of being questioned rather
> than explained.
> I spend most of my time looking for bugs, following my interest, so
> I’m more familiar
> with bug patterns than with the deeper details of the subsystem. That
> means I can
> make mistakes. I’m willing to listen to feedback and keep revising the
> patch until it’s in
> good shape, but the pressure sometimes makes me afraid to continue the
> conversation,
> worried about making more mistakes. Your patient explanation this time
> really means a
> lot to me.
> 
> Of course, I also fully understand that busy developers feel
> uncomfortable when seeing
> unclear or poorly written messages. I just wanted to express my thanks
> for taking the time
> to gently and patiently explain things clearly.
> 
> I’ll carefully rework the commit message following your suggestions to
> make the concurrency
> scenario and the reasoning clearer, and send a new patch once it’s
> ready. I’d appreciate it if
> you could take another look then.

While AI assistance is allowed, if this happens as a pattern, it is
misuse and eager way to credit without due. I.e., you've done 50%
of the work but ignored the human part.

I.e., you also must understand without AI assistance what
the patch does. If you do, it should not a problem to write
a commit message tht simply describes what it does.

To summarize:

1. Actual mistakes are fine and then patch just needs to be iterated.
2. Skipping work is not mistake and it is not cool.

Hope this clears things up.

> 
> Best regards,
> Cen Zhang

Br, Jarkko

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

[Cen Zhang]

Jarkko Sakkinen <jarkko@kernel.org> 于2026年10月12日周一 21:49写道：
>
> While AI assistance is allowed, if this happens as a pattern, it is
> misuse and eager way to credit without due. I.e., you've done 50%
> of the work but ignored the human part.
>
> I.e., you also must understand without AI assistance what
> the patch does. If you do, it should not a problem to write
> a commit message tht simply describes what it does.
>
> To summarize:
>
> 1. Actual mistakes are fine and then patch just needs to be iterated.
> 2. Skipping work is not mistake and it is not cool.
>
> Hope this clears things up.
>

Thanks for the guidance. I’ll keep it in mind.

Best regards,
Cen