Re: [PATCH] keys: serialise instantiation against keyctl_chown_key

[sashiko-bot@kernel.org]

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