[PATCH] keys: serialise instantiation against keyctl_chown_key

[PATCH] keys: serialise instantiation against keyctl_chown_key

[Sinan Turunc]

keyctl_chown_key() replaces key->user under key->sem, but the
instantiation path reads key->user without it: __key_instantiate_and_link()
and key_reject_and_link() bump key->user->nikeys under
key_construction_mutex only, and key_payload_reserve(), called from the
type's instantiate op, reads key->user once for spin_lock_irqsave() and
again for spin_unlock_irqrestore().

A chown of an under-construction key that lands between those two reads
locks the old owner's quota spinlock and unlocks the new owner's.  The
paravirt unlock slow path reports the unheld unlock and the old lock is
never released, so the next quota operation on that uid spins with
interrupts disabled:

  pvqspinlock: lock 0xffff88800f94dc38 has corrupted value 0x0!
  WARNING: CPU: 2 PID: 215 at kernel/locking/qspinlock_paravirt.h:504 __pv_queued_spin_unlock_slowpath+0x1dc/0x2e0
  Call Trace:
   _raw_spin_unlock_irqrestore+0xe/0x30
   key_payload_reserve+0x230/0x300
   generic_key_instantiate+0x47/0x230
   __key_instantiate_and_link+0xbd/0x310
   key_instantiate_and_link+0x3f0/0x490
   keyctl_instantiate_key_common+0x246/0x410

The same unsynchronised read lets the instantiator touch the old
key_user after the chown has dropped the last reference to it.

Hold key->sem for write across the instantiate op and the owner-record
updates so that instantiation and chown serialise on the lock that
already protects key->user.  key->sem nests inside key_construction_mutex
and the destination keyring's semaphore here; nothing takes the
construction mutex under a key's semaphore.  The acquisition is
annotated as nested because a keyring instantiated into a keyring shares
the destination's lock class; the key under construction cannot be a
destination itself, so no cycle exists.

Reproducer: a request_key() helper that chowns the target key between
two uids from one thread while instantiating it from another, as root.
Triggers within tens of milliseconds on 6.12.104 and on mainline.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Sinan Turunc <enscrpt@pm.me>
---
An AI tool helped with the analysis and the patch. I reviewed and
tested it myself: the unpatched kernel hangs within 40 ms, the patched
one ran the same workload for 60 s with lockdep on and no report.

 security/keys/key.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -439,7 +439,13 @@
 
 	mutex_lock(&key_construction_mutex);
 
-	/* can't instantiate twice */
+	/* can't instantiate twice; key->sem keeps key->user stable against
+	 * keyctl_chown_key() for the instantiate op and the quota update.
+	 * Nested: the caller may hold the destination keyring's sem and the
+	 * key under construction may be a keyring too, but it cannot be
+	 * anyone's destination yet, so there is no cycle.
+	 */
+	down_write_nested(&key->sem, 1);
 	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* instantiate the key */
 		ret = key->type->instantiate(key, prep);
@@ -469,6 +475,7 @@
 				key_set_expiry(key, prep->expiry);
 		}
 	}
+	up_write(&key->sem);
 
 	mutex_unlock(&key_construction_mutex);
 
@@ -600,7 +607,8 @@
 
 	mutex_lock(&key_construction_mutex);
 
-	/* can't instantiate twice */
+	/* can't instantiate twice; see __key_instantiate_and_link() */
+	down_write_nested(&key->sem, 1);
 	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
@@ -621,6 +629,7 @@
 		if (authkey)
 			key_invalidate(authkey);
 	}
+	up_write(&key->sem);
 
 	mutex_unlock(&key_construction_mutex);
 
-- 
2.56.0

Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[Jarkko Sakkinen]

On Sun, Nov 15, 2026 at 12:17:40PM +0000, Sinan Turunc wrote:
> keyctl_chown_key() replaces key->user under key->sem, but the
> instantiation path reads key->user without it: __key_instantiate_and_link()
> and key_reject_and_link() bump key->user->nikeys under
> key_construction_mutex only, and key_payload_reserve(), called from the
> type's instantiate op, reads key->user once for spin_lock_irqsave() and
> again for spin_unlock_irqrestore().
> 
> A chown of an under-construction key that lands between those two reads
> locks the old owner's quota spinlock and unlocks the new owner's.  The
> paravirt unlock slow path reports the unheld unlock and the old lock is
> never released, so the next quota operation on that uid spins with
> interrupts disabled:
> 
>   pvqspinlock: lock 0xffff88800f94dc38 has corrupted value 0x0!
>   WARNING: CPU: 2 PID: 215 at kernel/locking/qspinlock_paravirt.h:504 __pv_queued_spin_unlock_slowpath+0x1dc/0x2e0
>   Call Trace:
>    _raw_spin_unlock_irqrestore+0xe/0x30
>    key_payload_reserve+0x230/0x300
>    generic_key_instantiate+0x47/0x230
>    __key_instantiate_and_link+0xbd/0x310
>    key_instantiate_and_link+0x3f0/0x490
>    keyctl_instantiate_key_common+0x246/0x410
> 
> The same unsynchronised read lets the instantiator touch the old
> key_user after the chown has dropped the last reference to it.
> 
> Hold key->sem for write across the instantiate op and the owner-record
> updates so that instantiation and chown serialise on the lock that
> already protects key->user.  key->sem nests inside key_construction_mutex
> and the destination keyring's semaphore here; nothing takes the
> construction mutex under a key's semaphore.  The acquisition is
> annotated as nested because a keyring instantiated into a keyring shares
> the destination's lock class; the key under construction cannot be a
> destination itself, so no cycle exists.
> 
> Reproducer: a request_key() helper that chowns the target key between
> two uids from one thread while instantiating it from another, as root.
> Triggers within tens of milliseconds on 6.12.104 and on mainline.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sinan Turunc <enscrpt@pm.me>
> ---
> An AI tool helped with the analysis and the patch. I reviewed and
> tested it myself: the unpatched kernel hangs within 40 ms, the patched
> one ran the same workload for 60 s with lockdep on and no report.
> 
>  security/keys/key.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -439,7 +439,13 @@
>  
>  	mutex_lock(&key_construction_mutex);
>  
> -	/* can't instantiate twice */
> +	/* can't instantiate twice; key->sem keeps key->user stable against
> +	 * keyctl_chown_key() for the instantiate op and the quota update.
> +	 * Nested: the caller may hold the destination keyring's sem and the
> +	 * key under construction may be a keyring too, but it cannot be
> +	 * anyone's destination yet, so there is no cycle.
> +	 */

:-(

> +	down_write_nested(&key->sem, 1);
>  	if (key->state == KEY_IS_UNINSTANTIATED) {
>  		/* instantiate the key */
>  		ret = key->type->instantiate(key, prep);
> @@ -469,6 +475,7 @@
>  				key_set_expiry(key, prep->expiry);
>  		}
>  	}
> +	up_write(&key->sem);
>  
>  	mutex_unlock(&key_construction_mutex);
>  
> @@ -600,7 +607,8 @@
>  
>  	mutex_lock(&key_construction_mutex);
>  
> -	/* can't instantiate twice */
> +	/* can't instantiate twice; see __key_instantiate_and_link() */
> +	down_write_nested(&key->sem, 1);
>  	if (key->state == KEY_IS_UNINSTANTIATED) {
>  		/* mark the key as being negatively instantiated */
>  		atomic_inc(&key->user->nikeys);
> @@ -621,6 +629,7 @@
>  		if (authkey)
>  			key_invalidate(authkey);
>  	}
> +	up_write(&key->sem);
>  
>  	mutex_unlock(&key_construction_mutex);
>  
> -- 
> 2.56.0
> 

Please review the LLM generated patch yourself at first and write
a commit message thta you can understand yourself.

Plus, remove spurious inline comment changes. I'm only then going
to review this.

Br, Jarkko

Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[Jarkko Sakkinen]

On Sun, Nov 15, 2026 at 02:30:23PM +0200, Jarkko Sakkinen wrote:
> Please review the LLM generated patch yourself at first and write
> a commit message thta you can understand yourself.
> 
> Plus, remove spurious inline comment changes. I'm only then going
> to review this.

I.e., I have serious concerns that you might not know what you're
doing. Well-formed commit is the way to get evidence on that.

br, Jarkko

Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[Jarkko Sakkinen]

On Sun, Nov 15, 2026 at 02:33:36PM +0200, Jarkko Sakkinen wrote:
> On Sun, Nov 15, 2026 at 02:30:23PM +0200, Jarkko Sakkinen wrote:
> > Please review the LLM generated patch yourself at first and write
> > a commit message thta you can understand yourself.
> > 
> > Plus, remove spurious inline comment changes. I'm only then going
> > to review this.
> 
> I.e., I have serious concerns that you might not know what you're
> doing. Well-formed commit is the way to get evidence on that.

E.g., if I pick and write proper explanation instead of slop,
should I call it my own?

To have a review process, there must be two who think. Otherwise,
it is just me figuring out all of this; and LLM. You have no role.

This is also why it is important to write proper commits.

Br, Jarkko

Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[Jarkko Sakkinen]

On Sun, Nov 15, 2026 at 02:36:25PM +0200, Jarkko Sakkinen wrote:
> On Sun, Nov 15, 2026 at 02:33:36PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Nov 15, 2026 at 02:30:23PM +0200, Jarkko Sakkinen wrote:
> > > Please review the LLM generated patch yourself at first and write
> > > a commit message thta you can understand yourself.
> > > 
> > > Plus, remove spurious inline comment changes. I'm only then going
> > > to review this.
> > 
> > I.e., I have serious concerns that you might not know what you're
> > doing. Well-formed commit is the way to get evidence on that.
> 
> E.g., if I pick and write proper explanation instead of slop,
> should I call it my own?
> 
> To have a review process, there must be two who think. Otherwise,
> it is just me figuring out all of this; and LLM. You have no role.
> 
> This is also why it is important to write proper commits.

Finally, while not knowing if this is the case, it is irresponsible
as hell to send patches to subsystems like keyrings *if* you don't
understand yourself exactly what you are doing.

Leaf driver mixes up only that leave driver; an uncosidered fix to
a subsystem that is always on  side-effects can do much worse.

Have you considered this?

I'll hold for for v2 :-) Just do the work properly. That's all.

Br, Jarkko