[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

Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] keys: lockdep cycle via key->sem subclass 1 in
  __key_instantiate_and_link()
- [Low] keys: unresolvable Fixes trailer commit SHA 1da177e4c3f4
--

commit 63d8e840b5009e3c4507a5b0e1379f0f635dfd4b
Author: Sinan Turunc <enscrpt@pm.me>

> keys: serialise instantiation against keyctl_chown_key
>
> 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>

[Severity: Low]
The commit sha in the Fixes: trailer cannot be resolved in the repository
(it points to the initial git import).

> diff --git a/security/keys/key.c b/security/keys/key.c
> index a438c4508595d..afcf9fe04fb24 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -440,7 +440,13 @@ static int __key_instantiate_and_link(struct key *key,
>  
>  	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);

[Severity: Medium]
Does using subclass 1 for key->sem in __key_instantiate_and_link() and
key_reject_and_link() trigger a lockdep circular dependency report against
__key_move_lock()?

All keyrings share &key_type_keyring.lock_class.  When instantiating a
keyring into a destination keyring, __key_link_lock() takes the destination
keyring->sem (subclass 0) and then keyring_serialise_link_lock before
__key_instantiate_and_link() takes key->sem at subclass 1.  Moving a
keyring via key_move() -> __key_move_lock() takes the second keyring's
semaphore at subclass 1 before taking keyring_serialise_link_lock:

CPU0                                  CPU1
----                                  ----
down_write(&keyring->sem)
mutex_lock(&keyring_serialise_link_lock)
                                      down_write(&l_keyring->sem)
                                      down_write_nested(&u_keyring->sem, 1)
                                      mutex_lock(&keyring_serialise_link_lock)
mutex_lock(&key_construction_mutex)
down_write_nested(&key->sem, 1)

 *** DEADLOCK ***

CPU0: key_instantiate_and_link() -> __key_link_lock() /
      __key_instantiate_and_link()
CPU1: keyctl_keyring_move() -> key_move() -> __key_move_lock()

Although an uninstantiated keyring cannot be looked up by
keyctl_keyring_move() at runtime, reusing subclass 1 here inverts the
lockdep order between (&key_type_keyring.lock_class, 1) and
keyring_serialise_link_lock.  Using a distinct subclass (such as 2) for
key->sem in __key_instantiate_and_link() and key_reject_and_link() would
avoid the collision.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115121722.206-1-enscrpt@pm.me?part=1