Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[Jarkko Sakkinen <jarkko@kernel.org>]

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