[PATCH v3 1/2] keys: Protect key_user lifetime during ownership changes

[PATCH v3 1/2] keys: Protect key_user lifetime during ownership changes

[Chengfeng Ye]

keyctl_chown_key() replaces key->user under key->sem and drops the
reference to the previous owner after releasing the semaphore. Readers
which do not hold that semaphore can still be using the previous owner
when key_user_put() frees it.

For example, namespace filtering in /proc/keys can race with chown:

  /proc/keys reader                 keyctl_chown_key()
  user = key->user
                                    key->user = newowner
                                    key_user_put(old)
                                      kfree(old)
  read user->uid

An earlier instrumented run reported:

  BUG: KASAN: slab-use-after-free in proc_keys_start+0x353/0x440
  Read of size 4 at addr ffff8881128abbc0 by task poc/88
  Call Trace:
   proc_keys_start+0x353/0x440
   seq_read_iter+0x25d/0x1190
   proc_reg_read_iter+0x19e/0x260
  Allocated by task 86:
   key_user_lookup+0x1b4/0x540
   keyctl_chown_key+0x3cc/0xbf0
  Freed by task 87:
   kfree+0x149/0x330
   keyctl_chown_key+0x7b0/0xbf0

Serialize pointer replacement and the affected readers with key_user_lock,
which already protects final key_user removal. Take the lock explicitly
while /proc/keys and find_keyring_by_name() copy the owner's UID. Retain
the quota-owner UID rather than substituting key->uid, since they may
differ for thread keyrings.

Also hold the lock across owner accesses in key_payload_reserve() and
across the instantiated-key count increments. Instantiation need not hold
the target key's semaphore, so these paths need the same lifetime
protection. Leave key-state publication and the chown accounting transfer
outside the new critical sections for the separate accounting fix.

Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
Changes in v3:
- Split from v2 as patch 1/2; see the cover letter for the full split.
- Rebase onto current mainline and retain explicit reader-side locking.

v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/

 security/keys/key.c     | 13 +++++++++++--
 security/keys/keyctl.c  |  2 ++
 security/keys/keyring.c |  7 ++++++-
 security/keys/proc.c    | 15 +++++++++++++--
 4 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index b34a64d81d47..d0d583194b05 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -21,6 +21,7 @@ struct rb_root		key_serial_tree; /* tree of keys indexed by serial */
 DEFINE_SPINLOCK(key_serial_lock);
 
 struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
+/* Protects key_user_tree and key ownership changes. */
 DEFINE_SPINLOCK(key_user_lock);
 
 unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
@@ -380,9 +381,12 @@ int key_payload_reserve(struct key *key, size_t datalen)
 
 	/* contemplate the quota adjustment */
 	if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
-		unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
-			key_quota_root_maxbytes : key_quota_maxbytes;
 		unsigned long flags;
+		unsigned int maxbytes;
+
+		spin_lock(&key_user_lock);
+		maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
+			key_quota_root_maxbytes : key_quota_maxbytes;
 
 		spin_lock_irqsave(&key->user->lock, flags);
 
@@ -396,6 +400,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
 			key->quotalen += delta;
 		}
 		spin_unlock_irqrestore(&key->user->lock, flags);
+		spin_unlock(&key_user_lock);
 	}
 
 	/* change the recorded data length if that didn't generate an error */
@@ -447,7 +452,9 @@ static int __key_instantiate_and_link(struct key *key,
 
 		if (ret == 0) {
 			/* mark the key as being instantiated */
+			spin_lock(&key_user_lock);
 			atomic_inc(&key->user->nikeys);
+			spin_unlock(&key_user_lock);
 			mark_key_instantiated(key, 0);
 			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
 
@@ -604,7 +611,9 @@ int key_reject_and_link(struct key *key,
 	/* can't instantiate twice */
 	if (key->state == KEY_IS_UNINSTANTIATED) {
 		/* mark the key as being negatively instantiated */
+		spin_lock(&key_user_lock);
 		atomic_inc(&key->user->nikeys);
+		spin_unlock(&key_user_lock);
 		mark_key_instantiated(key, -error);
 		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
 		key_set_expiry(key, ktime_get_real_seconds() + timeout);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index d14ace88e529..c17924609317 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1036,8 +1036,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 			atomic_inc(&newowner->nikeys);
 		}
 
+		spin_lock(&key_user_lock);
 		zapowner = key->user;
 		key->user = newowner;
+		spin_unlock(&key_user_lock);
 		key->uid = uid;
 	}
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 15bf4af8f282..5943e8b48c0f 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1148,6 +1148,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 {
 	struct user_namespace *ns = current_user_ns();
 	struct key *keyring;
+	kuid_t uid;
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
@@ -1158,7 +1159,11 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 	 * grants Search permission and that hasn't been revoked
 	 */
 	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
-		if (!kuid_has_mapping(ns, keyring->user->uid))
+		spin_lock(&key_user_lock);
+		uid = keyring->user->uid;
+		spin_unlock(&key_user_lock);
+
+		if (!kuid_has_mapping(ns, uid))
 			continue;
 
 		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 4f4e2c1824f1..e507c500c068 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -68,7 +68,13 @@ static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
 	n = rb_next(n);
 	while (n) {
 		struct key *key = rb_entry(n, struct key, serial_node);
-		if (kuid_has_mapping(user_ns, key->user->uid))
+		kuid_t uid;
+
+		spin_lock(&key_user_lock);
+		uid = key->user->uid;
+		spin_unlock(&key_user_lock);
+
+		if (kuid_has_mapping(user_ns, uid))
 			break;
 		n = rb_next(n);
 	}
@@ -80,6 +86,7 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
 	struct user_namespace *user_ns = seq_user_ns(p);
 	struct rb_node *n = key_serial_tree.rb_node;
 	struct key *minkey = NULL;
+	kuid_t uid;
 
 	while (n) {
 		struct key *key = rb_entry(n, struct key, serial_node);
@@ -100,7 +107,11 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
 		return NULL;
 
 	for (;;) {
-		if (kuid_has_mapping(user_ns, minkey->user->uid))
+		spin_lock(&key_user_lock);
+		uid = minkey->user->uid;
+		spin_unlock(&key_user_lock);
+
+		if (kuid_has_mapping(user_ns, uid))
 			return minkey;
 		n = rb_next(&minkey->serial_node);
 		if (!n)
-- 
2.43.0

Re: [PATCH v3 1/2] keys: Protect key_user lifetime during ownership changes

[Jarkko Sakkinen]

On Mon, Sep 28, 2026 at 12:25:27AM +0800, Chengfeng Ye wrote:
> keyctl_chown_key() replaces key->user under key->sem and drops the
> reference to the previous owner after releasing the semaphore. Readers
> which do not hold that semaphore can still be using the previous owner
> when key_user_put() frees it.
> 
> For example, namespace filtering in /proc/keys can race with chown:
> 
>   /proc/keys reader                 keyctl_chown_key()
>   user = key->user
>                                     key->user = newowner
>                                     key_user_put(old)
>                                       kfree(old)
>   read user->uid
> 
> An earlier instrumented run reported:
> 
>   BUG: KASAN: slab-use-after-free in proc_keys_start+0x353/0x440
>   Read of size 4 at addr ffff8881128abbc0 by task poc/88
>   Call Trace:
>    proc_keys_start+0x353/0x440
>    seq_read_iter+0x25d/0x1190
>    proc_reg_read_iter+0x19e/0x260
>   Allocated by task 86:
>    key_user_lookup+0x1b4/0x540
>    keyctl_chown_key+0x3cc/0xbf0
>   Freed by task 87:
>    kfree+0x149/0x330
>    keyctl_chown_key+0x7b0/0xbf0
> 
> Serialize pointer replacement and the affected readers with key_user_lock,
> which already protects final key_user removal. Take the lock explicitly
> while /proc/keys and find_keyring_by_name() copy the owner's UID. Retain
> the quota-owner UID rather than substituting key->uid, since they may
> differ for thread keyrings.
> 
> Also hold the lock across owner accesses in key_payload_reserve() and
> across the instantiated-key count increments. Instantiation need not hold
> the target key's semaphore, so these paths need the same lifetime
> protection. Leave key-state publication and the chown accounting transfer
> outside the new critical sections for the separate accounting fix.
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
> Changes in v3:
> - Split from v2 as patch 1/2; see the cover letter for the full split.
> - Rebase onto current mainline and retain explicit reader-side locking.
> 
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> 
>  security/keys/key.c     | 13 +++++++++++--
>  security/keys/keyctl.c  |  2 ++
>  security/keys/keyring.c |  7 ++++++-
>  security/keys/proc.c    | 15 +++++++++++++--
>  4 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index b34a64d81d47..d0d583194b05 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -21,6 +21,7 @@ struct rb_root		key_serial_tree; /* tree of keys indexed by serial */
>  DEFINE_SPINLOCK(key_serial_lock);
>  
>  struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
> +/* Protects key_user_tree and key ownership changes. */
>  DEFINE_SPINLOCK(key_user_lock);
>  
>  unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
> @@ -380,9 +381,12 @@ int key_payload_reserve(struct key *key, size_t datalen)
>  
>  	/* contemplate the quota adjustment */
>  	if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> -		unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> -			key_quota_root_maxbytes : key_quota_maxbytes;
>  		unsigned long flags;
> +		unsigned int maxbytes;
> +
> +		spin_lock(&key_user_lock);
> +		maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> +			key_quota_root_maxbytes : key_quota_maxbytes;
>  
>  		spin_lock_irqsave(&key->user->lock, flags);
>  
> @@ -396,6 +400,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
>  			key->quotalen += delta;
>  		}
>  		spin_unlock_irqrestore(&key->user->lock, flags);
> +		spin_unlock(&key_user_lock);
>  	}
>  
>  	/* change the recorded data length if that didn't generate an error */
> @@ -447,7 +452,9 @@ static int __key_instantiate_and_link(struct key *key,
>  
>  		if (ret == 0) {
>  			/* mark the key as being instantiated */
> +			spin_lock(&key_user_lock);
>  			atomic_inc(&key->user->nikeys);
> +			spin_unlock(&key_user_lock);
>  			mark_key_instantiated(key, 0);
>  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>  
> @@ -604,7 +611,9 @@ int key_reject_and_link(struct key *key,
>  	/* can't instantiate twice */
>  	if (key->state == KEY_IS_UNINSTANTIATED) {
>  		/* mark the key as being negatively instantiated */
> +		spin_lock(&key_user_lock);
>  		atomic_inc(&key->user->nikeys);
> +		spin_unlock(&key_user_lock);
>  		mark_key_instantiated(key, -error);
>  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529..c17924609317 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1036,8 +1036,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  			atomic_inc(&newowner->nikeys);
>  		}
>  
> +		spin_lock(&key_user_lock);
>  		zapowner = key->user;
>  		key->user = newowner;
> +		spin_unlock(&key_user_lock);
>  		key->uid = uid;
>  	}
>  
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 15bf4af8f282..5943e8b48c0f 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1148,6 +1148,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
>  {
>  	struct user_namespace *ns = current_user_ns();
>  	struct key *keyring;
> +	kuid_t uid;
>  
>  	if (!name)
>  		return ERR_PTR(-EINVAL);
> @@ -1158,7 +1159,11 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
>  	 * grants Search permission and that hasn't been revoked
>  	 */
>  	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
> -		if (!kuid_has_mapping(ns, keyring->user->uid))
> +		spin_lock(&key_user_lock);
> +		uid = keyring->user->uid;
> +		spin_unlock(&key_user_lock);
> +
> +		if (!kuid_has_mapping(ns, uid))
>  			continue;
>  
>  		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index 4f4e2c1824f1..e507c500c068 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -68,7 +68,13 @@ static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
>  	n = rb_next(n);
>  	while (n) {
>  		struct key *key = rb_entry(n, struct key, serial_node);
> -		if (kuid_has_mapping(user_ns, key->user->uid))
> +		kuid_t uid;
> +
> +		spin_lock(&key_user_lock);
> +		uid = key->user->uid;
> +		spin_unlock(&key_user_lock);
> +
> +		if (kuid_has_mapping(user_ns, uid))
>  			break;
>  		n = rb_next(n);
>  	}
> @@ -80,6 +86,7 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
>  	struct user_namespace *user_ns = seq_user_ns(p);
>  	struct rb_node *n = key_serial_tree.rb_node;
>  	struct key *minkey = NULL;
> +	kuid_t uid;
>  
>  	while (n) {
>  		struct key *key = rb_entry(n, struct key, serial_node);
> @@ -100,7 +107,11 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
>  		return NULL;
>  
>  	for (;;) {
> -		if (kuid_has_mapping(user_ns, minkey->user->uid))
> +		spin_lock(&key_user_lock);
> +		uid = minkey->user->uid;
> +		spin_unlock(&key_user_lock);
> +
> +		if (kuid_has_mapping(user_ns, uid))
>  			return minkey;
>  		n = rb_next(&minkey->serial_node);
>  		if (!n)
> -- 
> 2.43.0
> 

Thanks, great work!

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH v3 1/2] keys: Protect key_user lifetime during ownership changes

[Jarkko Sakkinen]

While preparing PR some changes in this patch started to bother me so
let's do a sanity check.

On Mon, Sep 28, 2026 at 12:25:27AM +0800, Chengfeng Ye wrote:
> keyctl_chown_key() replaces key->user under key->sem and drops the
> reference to the previous owner after releasing the semaphore. Readers
> which do not hold that semaphore can still be using the previous owner
> when key_user_put() frees it.
> 
> For example, namespace filtering in /proc/keys can race with chown:
> 
>   /proc/keys reader                 keyctl_chown_key()
>   user = key->user
>                                     key->user = newowner
>                                     key_user_put(old)
>                                       kfree(old)
>   read user->uid
> 
> An earlier instrumented run reported:
> 
>   BUG: KASAN: slab-use-after-free in proc_keys_start+0x353/0x440
>   Read of size 4 at addr ffff8881128abbc0 by task poc/88
>   Call Trace:
>    proc_keys_start+0x353/0x440
>    seq_read_iter+0x25d/0x1190
>    proc_reg_read_iter+0x19e/0x260
>   Allocated by task 86:
>    key_user_lookup+0x1b4/0x540
>    keyctl_chown_key+0x3cc/0xbf0
>   Freed by task 87:
>    kfree+0x149/0x330
>    keyctl_chown_key+0x7b0/0xbf0
> 
> Serialize pointer replacement and the affected readers with key_user_lock,
> which already protects final key_user removal. Take the lock explicitly
> while /proc/keys and find_keyring_by_name() copy the owner's UID. Retain
> the quota-owner UID rather than substituting key->uid, since they may
> differ for thread keyrings.
> 
> Also hold the lock across owner accesses in key_payload_reserve() and
> across the instantiated-key count increments. Instantiation need not hold
> the target key's semaphore, so these paths need the same lifetime
> protection. Leave key-state publication and the chown accounting transfer
> outside the new critical sections for the separate accounting fix.
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
> Changes in v3:
> - Split from v2 as patch 1/2; see the cover letter for the full split.
> - Rebase onto current mainline and retain explicit reader-side locking.
> 
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> 
>  security/keys/key.c     | 13 +++++++++++--
>  security/keys/keyctl.c  |  2 ++
>  security/keys/keyring.c |  7 ++++++-
>  security/keys/proc.c    | 15 +++++++++++++--
>  4 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index b34a64d81d47..d0d583194b05 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -21,6 +21,7 @@ struct rb_root		key_serial_tree; /* tree of keys indexed by serial */
>  DEFINE_SPINLOCK(key_serial_lock);
>  
>  struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
> +/* Protects key_user_tree and key ownership changes. */

This type of extras in bug fixes do not make any possible sense and set
an extra backporting cost.

>  DEFINE_SPINLOCK(key_user_lock);
>  
>  unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
> @@ -380,9 +381,12 @@ int key_payload_reserve(struct key *key, size_t datalen)
>  
>  	/* contemplate the quota adjustment */
>  	if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> -		unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> -			key_quota_root_maxbytes : key_quota_maxbytes;
>  		unsigned long flags;
> +		unsigned int maxbytes;
> +
> +		spin_lock(&key_user_lock);
> +		maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ?
> +			key_quota_root_maxbytes : key_quota_maxbytes;
>  
>  		spin_lock_irqsave(&key->user->lock, flags);
>  
> @@ -396,6 +400,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
>  			key->quotalen += delta;
>  		}
>  		spin_unlock_irqrestore(&key->user->lock, flags);
> +		spin_unlock(&key_user_lock);

I don't understand the necessity of key_user_lock here.

>  	}
>  
>  	/* change the recorded data length if that didn't generate an error */
> @@ -447,7 +452,9 @@ static int __key_instantiate_and_link(struct key *key,
>  
>  		if (ret == 0) {
>  			/* mark the key as being instantiated */
> +			spin_lock(&key_user_lock);
>  			atomic_inc(&key->user->nikeys);
> +			spin_unlock(&key_user_lock);
>  			mark_key_instantiated(key, 0);
>  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>  
> @@ -604,7 +611,9 @@ int key_reject_and_link(struct key *key,
>  	/* can't instantiate twice */
>  	if (key->state == KEY_IS_UNINSTANTIATED) {
>  		/* mark the key as being negatively instantiated */
> +		spin_lock(&key_user_lock);
>  		atomic_inc(&key->user->nikeys);
> +		spin_unlock(&key_user_lock);

atomic_inc() is wrapped under spinlock in the two changes above. Why
is that necessary?

>  		mark_key_instantiated(key, -error);
>  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d14ace88e529..c17924609317 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1036,8 +1036,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  			atomic_inc(&newowner->nikeys);
>  		}
>  
> +		spin_lock(&key_user_lock);
>  		zapowner = key->user;
>  		key->user = newowner;
> +		spin_unlock(&key_user_lock);

This is where I get it as you swap the key->user.

>  		key->uid = uid;
>  	}
>  
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 15bf4af8f282..5943e8b48c0f 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1148,6 +1148,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
>  {
>  	struct user_namespace *ns = current_user_ns();
>  	struct key *keyring;
> +	kuid_t uid;
>  
>  	if (!name)
>  		return ERR_PTR(-EINVAL);
> @@ -1158,7 +1159,11 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
>  	 * grants Search permission and that hasn't been revoked
>  	 */
>  	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
> -		if (!kuid_has_mapping(ns, keyring->user->uid))
> +		spin_lock(&key_user_lock);
> +		uid = keyring->user->uid;
> +		spin_unlock(&key_user_lock);

In this and similar site I do not get it. Why key->user->-lock could not
be instead?

> +
> +		if (!kuid_has_mapping(ns, uid))
>  			continue;
>  
>  		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index 4f4e2c1824f1..e507c500c068 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -68,7 +68,13 @@ static struct rb_node *key_serial_next(struct seq_file *p, struct rb_node *n)
>  	n = rb_next(n);
>  	while (n) {
>  		struct key *key = rb_entry(n, struct key, serial_node);
> -		if (kuid_has_mapping(user_ns, key->user->uid))
> +		kuid_t uid;
> +
> +		spin_lock(&key_user_lock);
> +		uid = key->user->uid;
> +		spin_unlock(&key_user_lock);
> +
> +		if (kuid_has_mapping(user_ns, uid))
>  			break;
>  		n = rb_next(n);

Isn't procfs change at least out of scope? It

>  	}
> @@ -80,6 +86,7 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
>  	struct user_namespace *user_ns = seq_user_ns(p);
>  	struct rb_node *n = key_serial_tree.rb_node;
>  	struct key *minkey = NULL;
> +	kuid_t uid;
>  
>  	while (n) {
>  		struct key *key = rb_entry(n, struct key, serial_node);
> @@ -100,7 +107,11 @@ static struct key *find_ge_key(struct seq_file *p, key_serial_t id)
>  		return NULL;
>  
>  	for (;;) {
> -		if (kuid_has_mapping(user_ns, minkey->user->uid))
> +		spin_lock(&key_user_lock);
> +		uid = minkey->user->uid;
> +		spin_unlock(&key_user_lock);
> +
> +		if (kuid_has_mapping(user_ns, uid))
>  			return minkey;
>  		n = rb_next(&minkey->serial_node);
>  		if (!n)
> -- 
> 2.43.0
> 

By putting global lock every possible site the bug does get fixed but
nothing really makes the case that this is ultimately least intrusive
way to fix this.

I could fix almost any possible race condition if there was a preference
to use global lock for everything. I'm neither sure whether this is a
superset change that covers the bug fix or not.

Br, Jarkko

Br, Jarkko

[PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Chengfeng Ye]

Protecting individual accesses to key->user does not make ownership
transfers atomic with accounting updates. keyctl_chown_key() holds
key->sem, but instantiation is serialized by key_construction_mutex and
need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
an uninstantiated key.

The instantiated-key count can therefore be charged to the wrong owner:

  instantiate                       keyctl_chown_key()
  lock key_user_lock
  increment old->nikeys
  unlock key_user_lock
                                    observe KEY_IS_UNINSTANTIATED
                                    skip the nikeys transfer
                                    replace key->user
  mark key instantiated

The key becomes instantiated under the new owner while the increment
remains with the old owner. Negative instantiation has the same race.

Quota reservation can likewise run between charging the new owner and
replacing key->user. It then adjusts the old owner's quota and changes
key->quotalen while chown is transferring that quota burden.

Extend the key_user_lock critical section in keyctl_chown_key() across
the quota and key-count transfers, state check, and owner replacement.
Also extend the instantiation critical sections across the state update,
so chown observes the count increment and instantiated state together.
The existing per-user quota locks continue to serialize quota changes
against other keys owned by the same user.

Keep allocations, notifications and reference release outside
key_user_lock, and release it on the quota-overrun path.

Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
Changes in v3:
- Split from v2 as patch 2/2; see the cover letter for the full split.
- Rebase onto current mainline and retain explicit reader-side locking.

v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/

 security/keys/key.c    | 4 ++--
 security/keys/keyctl.c | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index d0d583194b05..54c675b3b58d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
 			/* mark the key as being instantiated */
 			spin_lock(&key_user_lock);
 			atomic_inc(&key->user->nikeys);
-			spin_unlock(&key_user_lock);
 			mark_key_instantiated(key, 0);
+			spin_unlock(&key_user_lock);
 			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
 
 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
@@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
 		/* mark the key as being negatively instantiated */
 		spin_lock(&key_user_lock);
 		atomic_inc(&key->user->nikeys);
-		spin_unlock(&key_user_lock);
 		mark_key_instantiated(key, -error);
+		spin_unlock(&key_user_lock);
 		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
 		key_set_expiry(key, ktime_get_real_seconds() + timeout);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index c17924609317..83a9575b084e 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 		if (!newowner)
 			goto error_put;
 
+		spin_lock(&key_user_lock);
+
 		/* transfer the quota burden to the new user */
 		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
 			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
@@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 			atomic_inc(&newowner->nikeys);
 		}
 
-		spin_lock(&key_user_lock);
 		zapowner = key->user;
 		key->user = newowner;
-		spin_unlock(&key_user_lock);
 		key->uid = uid;
+		spin_unlock(&key_user_lock);
 	}
 
 	/* change the GID */
@@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 
 quota_overrun:
 	spin_unlock_irqrestore(&newowner->lock, flags);
+	spin_unlock(&key_user_lock);
 	zapowner = newowner;
 	ret = -EDQUOT;
 	goto error_put;
-- 
2.43.0

Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Jarkko Sakkinen]

On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> Protecting individual accesses to key->user does not make ownership
> transfers atomic with accounting updates. keyctl_chown_key() holds
> key->sem, but instantiation is serialized by key_construction_mutex and
> need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> an uninstantiated key.
> 
> The instantiated-key count can therefore be charged to the wrong owner:
> 
>   instantiate                       keyctl_chown_key()
>   lock key_user_lock
>   increment old->nikeys
>   unlock key_user_lock
>                                     observe KEY_IS_UNINSTANTIATED
>                                     skip the nikeys transfer
>                                     replace key->user
>   mark key instantiated
> 
> The key becomes instantiated under the new owner while the increment
> remains with the old owner. Negative instantiation has the same race.
> 
> Quota reservation can likewise run between charging the new owner and
> replacing key->user. It then adjusts the old owner's quota and changes
> key->quotalen while chown is transferring that quota burden.
> 
> Extend the key_user_lock critical section in keyctl_chown_key() across
> the quota and key-count transfers, state check, and owner replacement.
> Also extend the instantiation critical sections across the state update,
> so chown observes the count increment and instantiated state together.
> The existing per-user quota locks continue to serialize quota changes
> against other keys owned by the same user.
> 
> Keep allocations, notifications and reference release outside
> key_user_lock, and release it on the quota-overrun path.
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
> Changes in v3:
> - Split from v2 as patch 2/2; see the cover letter for the full split.
> - Rebase onto current mainline and retain explicit reader-side locking.
> 
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> 
>  security/keys/key.c    | 4 ++--
>  security/keys/keyctl.c | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index d0d583194b05..54c675b3b58d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
>  			/* mark the key as being instantiated */
>  			spin_lock(&key_user_lock);
>  			atomic_inc(&key->user->nikeys);
> -			spin_unlock(&key_user_lock);
>  			mark_key_instantiated(key, 0);
> +			spin_unlock(&key_user_lock);
>  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>  
>  			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
>  		/* mark the key as being negatively instantiated */
>  		spin_lock(&key_user_lock);
>  		atomic_inc(&key->user->nikeys);
> -		spin_unlock(&key_user_lock);
>  		mark_key_instantiated(key, -error);
> +		spin_unlock(&key_user_lock);
>  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
>  
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index c17924609317..83a9575b084e 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  		if (!newowner)
>  			goto error_put;
>  
> +		spin_lock(&key_user_lock);
> +
>  		/* transfer the quota burden to the new user */
>  		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>  			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> @@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  			atomic_inc(&newowner->nikeys);
>  		}
>  
> -		spin_lock(&key_user_lock);
>  		zapowner = key->user;
>  		key->user = newowner;
> -		spin_unlock(&key_user_lock);
>  		key->uid = uid;
> +		spin_unlock(&key_user_lock);
>  	}
>  
>  	/* change the GID */
> @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  
>  quota_overrun:
>  	spin_unlock_irqrestore(&newowner->lock, flags);
> +	spin_unlock(&key_user_lock);
>  	zapowner = newowner;
>  	ret = -EDQUOT;
>  	goto error_put;
> -- 
> 2.43.0
> 


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Jarkko Sakkinen]

On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> Protecting individual accesses to key->user does not make ownership
> transfers atomic with accounting updates. keyctl_chown_key() holds
> key->sem, but instantiation is serialized by key_construction_mutex and
> need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> an uninstantiated key.
> 
> The instantiated-key count can therefore be charged to the wrong owner:
> 
>   instantiate                       keyctl_chown_key()
>   lock key_user_lock
>   increment old->nikeys
>   unlock key_user_lock
>                                     observe KEY_IS_UNINSTANTIATED
>                                     skip the nikeys transfer
>                                     replace key->user
>   mark key instantiated
> 
> The key becomes instantiated under the new owner while the increment
> remains with the old owner. Negative instantiation has the same race.
> 
> Quota reservation can likewise run between charging the new owner and
> replacing key->user. It then adjusts the old owner's quota and changes
> key->quotalen while chown is transferring that quota burden.
> 
> Extend the key_user_lock critical section in keyctl_chown_key() across
> the quota and key-count transfers, state check, and owner replacement.
> Also extend the instantiation critical sections across the state update,
> so chown observes the count increment and instantiated state together.
> The existing per-user quota locks continue to serialize quota changes
> against other keys owned by the same user.
> 
> Keep allocations, notifications and reference release outside
> key_user_lock, and release it on the quota-overrun path.
> 
> Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
> Changes in v3:
> - Split from v2 as patch 2/2; see the cover letter for the full split.
> - Rebase onto current mainline and retain explicit reader-side locking.
> 
> v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> 
>  security/keys/key.c    | 4 ++--
>  security/keys/keyctl.c | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index d0d583194b05..54c675b3b58d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
>  			/* mark the key as being instantiated */
>  			spin_lock(&key_user_lock);
>  			atomic_inc(&key->user->nikeys);
> -			spin_unlock(&key_user_lock);
>  			mark_key_instantiated(key, 0);
> +			spin_unlock(&key_user_lock);
>  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>  
>  			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
>  		/* mark the key as being negatively instantiated */
>  		spin_lock(&key_user_lock);
>  		atomic_inc(&key->user->nikeys);
> -		spin_unlock(&key_user_lock);
>  		mark_key_instantiated(key, -error);
> +		spin_unlock(&key_user_lock);
>  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
>  
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index c17924609317..83a9575b084e 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  		if (!newowner)
>  			goto error_put;
>  
> +		spin_lock(&key_user_lock);
> +
>  		/* transfer the quota burden to the new user */
>  		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>  			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> @@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  			atomic_inc(&newowner->nikeys);
>  		}
>  
> -		spin_lock(&key_user_lock);
>  		zapowner = key->user;
>  		key->user = newowner;
> -		spin_unlock(&key_user_lock);
>  		key->uid = uid;
> +		spin_unlock(&key_user_lock);
>  	}
>  
>  	/* change the GID */
> @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  
>  quota_overrun:
>  	spin_unlock_irqrestore(&newowner->lock, flags);
> +	spin_unlock(&key_user_lock);
>  	zapowner = newowner;
>  	ret = -EDQUOT;
>  	goto error_put;
> -- 
> 2.43.0
> 

I double-checked this patch too given the concerns on 1/2 but nope, this
does not raise similar concerns as every critical section is strictly
related to ownership change.

E.g., I can be sure that the granularity is where it should be and locks
are actually needed in the first place.

Thus, I'm still including this patch to my next PR, and drop the first
one.

Br, Jarkko

Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Jarkko Sakkinen]

On Mon, Oct 05, 2026 at 05:17:42AM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> > Protecting individual accesses to key->user does not make ownership
> > transfers atomic with accounting updates. keyctl_chown_key() holds
> > key->sem, but instantiation is serialized by key_construction_mutex and
> > need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> > an uninstantiated key.
> > 
> > The instantiated-key count can therefore be charged to the wrong owner:
> > 
> >   instantiate                       keyctl_chown_key()
> >   lock key_user_lock
> >   increment old->nikeys
> >   unlock key_user_lock
> >                                     observe KEY_IS_UNINSTANTIATED
> >                                     skip the nikeys transfer
> >                                     replace key->user
> >   mark key instantiated
> > 
> > The key becomes instantiated under the new owner while the increment
> > remains with the old owner. Negative instantiation has the same race.
> > 
> > Quota reservation can likewise run between charging the new owner and
> > replacing key->user. It then adjusts the old owner's quota and changes
> > key->quotalen while chown is transferring that quota burden.
> > 
> > Extend the key_user_lock critical section in keyctl_chown_key() across
> > the quota and key-count transfers, state check, and owner replacement.
> > Also extend the instantiation critical sections across the state update,
> > so chown observes the count increment and instantiated state together.
> > The existing per-user quota locks continue to serialize quota changes
> > against other keys owned by the same user.
> > 
> > Keep allocations, notifications and reference release outside
> > key_user_lock, and release it on the quota-overrun path.
> > 
> > Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > ---
> > Changes in v3:
> > - Split from v2 as patch 2/2; see the cover letter for the full split.
> > - Rebase onto current mainline and retain explicit reader-side locking.
> > 
> > v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> > 
> >  security/keys/key.c    | 4 ++--
> >  security/keys/keyctl.c | 6 ++++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index d0d583194b05..54c675b3b58d 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
> >  			/* mark the key as being instantiated */
> >  			spin_lock(&key_user_lock);
> >  			atomic_inc(&key->user->nikeys);
> > -			spin_unlock(&key_user_lock);
> >  			mark_key_instantiated(key, 0);
> > +			spin_unlock(&key_user_lock);
> >  			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
> >  
> >  			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> > @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
> >  		/* mark the key as being negatively instantiated */
> >  		spin_lock(&key_user_lock);
> >  		atomic_inc(&key->user->nikeys);
> > -		spin_unlock(&key_user_lock);
> >  		mark_key_instantiated(key, -error);
> > +		spin_unlock(&key_user_lock);
> >  		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
> >  		key_set_expiry(key, ktime_get_real_seconds() + timeout);
> >  
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index c17924609317..83a9575b084e 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> >  		if (!newowner)
> >  			goto error_put;
> >  
> > +		spin_lock(&key_user_lock);
> > +
> >  		/* transfer the quota burden to the new user */
> >  		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> >  			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> > @@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> >  			atomic_inc(&newowner->nikeys);
> >  		}
> >  
> > -		spin_lock(&key_user_lock);
> >  		zapowner = key->user;
> >  		key->user = newowner;
> > -		spin_unlock(&key_user_lock);
> >  		key->uid = uid;
> > +		spin_unlock(&key_user_lock);
> >  	}
> >  
> >  	/* change the GID */
> > @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> >  
> >  quota_overrun:
> >  	spin_unlock_irqrestore(&newowner->lock, flags);
> > +	spin_unlock(&key_user_lock);
> >  	zapowner = newowner;
> >  	ret = -EDQUOT;
> >  	goto error_put;
> > -- 
> > 2.43.0
> > 
> 
> I double-checked this patch too given the concerns on 1/2 but nope, this
> does not raise similar concerns as every critical section is strictly
> related to ownership change.
> 
> E.g., I can be sure that the granularity is where it should be and locks
> are actually needed in the first place.
> 
> Thus, I'm still including this patch to my next PR, and drop the first
> one.

By dropping 1/2 I found out that 2/2 key.c becomes:

diff --git a/security/keys/key.c b/security/keys/key.c
index a438c4508595..de63120c51ad 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -449,6 +449,7 @@ static int __key_instantiate_and_link(struct key *key,
 			/* mark the key as being instantiated */
 			atomic_inc(&key->user->nikeys);
 			mark_key_instantiated(key, 0);
+			spin_unlock(&key_user_lock);
 			notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);

 			if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
@@ -606,6 +607,7 @@ int key_reject_and_link(struct key *key,
 		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
 		mark_key_instantiated(key, -error);
+		spin_unlock(&key_user_lock);
 		notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
 		key_set_expiry(key, ktime_get_real_seconds() + timeout);

This type of interleaving should never happen in a patch series.

Please don't rush your changes like this in future.

Br, Jarkko

Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Chengfeng Ye]

On Mon, Oct 5, 2026 at 10:49 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Mon, Oct 05, 2026 at 05:17:42AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> > > Protecting individual accesses to key->user does not make ownership
> > > transfers atomic with accounting updates. keyctl_chown_key() holds
> > > key->sem, but instantiation is serialized by key_construction_mutex and
> > > need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> > > an uninstantiated key.
> > >
> > > The instantiated-key count can therefore be charged to the wrong owner:
> > >
> > >   instantiate                       keyctl_chown_key()
> > >   lock key_user_lock
> > >   increment old->nikeys
> > >   unlock key_user_lock
> > >                                     observe KEY_IS_UNINSTANTIATED
> > >                                     skip the nikeys transfer
> > >                                     replace key->user
> > >   mark key instantiated
> > >
> > > The key becomes instantiated under the new owner while the increment
> > > remains with the old owner. Negative instantiation has the same race.
> > >
> > > Quota reservation can likewise run between charging the new owner and
> > > replacing key->user. It then adjusts the old owner's quota and changes
> > > key->quotalen while chown is transferring that quota burden.
> > >
> > > Extend the key_user_lock critical section in keyctl_chown_key() across
> > > the quota and key-count transfers, state check, and owner replacement.
> > > Also extend the instantiation critical sections across the state update,
> > > so chown observes the count increment and instantiated state together.
> > > The existing per-user quota locks continue to serialize quota changes
> > > against other keys owned by the same user.
> > >
> > > Keep allocations, notifications and reference release outside
> > > key_user_lock, and release it on the quota-overrun path.
> > >
> > > Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > > ---
> > > Changes in v3:
> > > - Split from v2 as patch 2/2; see the cover letter for the full split.
> > > - Rebase onto current mainline and retain explicit reader-side locking.
> > >
> > > v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> > >
> > >  security/keys/key.c    | 4 ++--
> > >  security/keys/keyctl.c | 6 ++++--
> > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > index d0d583194b05..54c675b3b58d 100644
> > > --- a/security/keys/key.c
> > > +++ b/security/keys/key.c
> > > @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
> > >                     /* mark the key as being instantiated */
> > >                     spin_lock(&key_user_lock);
> > >                     atomic_inc(&key->user->nikeys);
> > > -                   spin_unlock(&key_user_lock);
> > >                     mark_key_instantiated(key, 0);
> > > +                   spin_unlock(&key_user_lock);
> > >                     notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
> > >
> > >                     if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> > > @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
> > >             /* mark the key as being negatively instantiated */
> > >             spin_lock(&key_user_lock);
> > >             atomic_inc(&key->user->nikeys);
> > > -           spin_unlock(&key_user_lock);
> > >             mark_key_instantiated(key, -error);
> > > +           spin_unlock(&key_user_lock);
> > >             notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
> > >             key_set_expiry(key, ktime_get_real_seconds() + timeout);
> > >
> > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > > index c17924609317..83a9575b084e 100644
> > > --- a/security/keys/keyctl.c
> > > +++ b/security/keys/keyctl.c
> > > @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > >             if (!newowner)
> > >                     goto error_put;
> > >
> > > +           spin_lock(&key_user_lock);
> > > +
> > >             /* transfer the quota burden to the new user */
> > >             if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> > >                     unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> > > @@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > >                     atomic_inc(&newowner->nikeys);
> > >             }
> > >
> > > -           spin_lock(&key_user_lock);
> > >             zapowner = key->user;
> > >             key->user = newowner;
> > > -           spin_unlock(&key_user_lock);
> > >             key->uid = uid;
> > > +           spin_unlock(&key_user_lock);
> > >     }
> > >
> > >     /* change the GID */
> > > @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > >
> > >  quota_overrun:
> > >     spin_unlock_irqrestore(&newowner->lock, flags);
> > > +   spin_unlock(&key_user_lock);
> > >     zapowner = newowner;
> > >     ret = -EDQUOT;
> > >     goto error_put;
> > > --
> > > 2.43.0
> > >
> >
> > I double-checked this patch too given the concerns on 1/2 but nope, this
> > does not raise similar concerns as every critical section is strictly
> > related to ownership change.
> >
> > E.g., I can be sure that the granularity is where it should be and locks
> > are actually needed in the first place.
> >
> > Thus, I'm still including this patch to my next PR, and drop the first
> > one.
>
> By dropping 1/2 I found out that 2/2 key.c becomes:
>
> diff --git a/security/keys/key.c b/security/keys/key.c
> index a438c4508595..de63120c51ad 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -449,6 +449,7 @@ static int __key_instantiate_and_link(struct key *key,
>                         /* mark the key as being instantiated */
>                         atomic_inc(&key->user->nikeys);
>                         mark_key_instantiated(key, 0);
> +                       spin_unlock(&key_user_lock);
>                         notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>
>                         if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> @@ -606,6 +607,7 @@ int key_reject_and_link(struct key *key,
>                 /* mark the key as being negatively instantiated */
>                 atomic_inc(&key->user->nikeys);
>                 mark_key_instantiated(key, -error);
> +               spin_unlock(&key_user_lock);
>                 notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
>                 key_set_expiry(key, ktime_get_real_seconds() + timeout);
>
> This type of interleaving should never happen in a patch series.
>
> Please don't rush your changes like this in future.
>
> Br, Jarkko

Sorry for the mistake, I should have carefully verified that the tree
remained valid at each step of the series.

I will rework the locking on 1/2 to inspect if there might be any
unnecessary coverage, and verify each intermediate commit
independently before sending a revised version for the patch series.

Best regards,
Chengfeng

Re: [PATCH v3 2/2] keys: Serialize ownership transfers with key accounting

[Jarkko Sakkinen]

On Mon, Oct 05, 2026 at 01:53:29PM +0800, Chengfeng Ye wrote:
> On Mon, Oct 5, 2026 at 10:49 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Mon, Oct 05, 2026 at 05:17:42AM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 28, 2026 at 12:25:28AM +0800, Chengfeng Ye wrote:
> > > > Protecting individual accesses to key->user does not make ownership
> > > > transfers atomic with accounting updates. keyctl_chown_key() holds
> > > > key->sem, but instantiation is serialized by key_construction_mutex and
> > > > need not hold that semaphore. KEY_LOOKUP_PARTIAL also permits chown of
> > > > an uninstantiated key.
> > > >
> > > > The instantiated-key count can therefore be charged to the wrong owner:
> > > >
> > > >   instantiate                       keyctl_chown_key()
> > > >   lock key_user_lock
> > > >   increment old->nikeys
> > > >   unlock key_user_lock
> > > >                                     observe KEY_IS_UNINSTANTIATED
> > > >                                     skip the nikeys transfer
> > > >                                     replace key->user
> > > >   mark key instantiated
> > > >
> > > > The key becomes instantiated under the new owner while the increment
> > > > remains with the old owner. Negative instantiation has the same race.
> > > >
> > > > Quota reservation can likewise run between charging the new owner and
> > > > replacing key->user. It then adjusts the old owner's quota and changes
> > > > key->quotalen while chown is transferring that quota burden.
> > > >
> > > > Extend the key_user_lock critical section in keyctl_chown_key() across
> > > > the quota and key-count transfers, state check, and owner replacement.
> > > > Also extend the instantiation critical sections across the state update,
> > > > so chown observes the count increment and instantiated state together.
> > > > The existing per-user quota locks continue to serialize quota changes
> > > > against other keys owned by the same user.
> > > >
> > > > Keep allocations, notifications and reference release outside
> > > > key_user_lock, and release it on the quota-overrun path.
> > > >
> > > > Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > > > ---
> > > > Changes in v3:
> > > > - Split from v2 as patch 2/2; see the cover letter for the full split.
> > > > - Rebase onto current mainline and retain explicit reader-side locking.
> > > >
> > > > v2: https://lore.kernel.org/r/20260904080940.575882-1-nicoyip.dev@gmail.com/
> > > >
> > > >  security/keys/key.c    | 4 ++--
> > > >  security/keys/keyctl.c | 6 ++++--
> > > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > > index d0d583194b05..54c675b3b58d 100644
> > > > --- a/security/keys/key.c
> > > > +++ b/security/keys/key.c
> > > > @@ -454,8 +454,8 @@ static int __key_instantiate_and_link(struct key *key,
> > > >                     /* mark the key as being instantiated */
> > > >                     spin_lock(&key_user_lock);
> > > >                     atomic_inc(&key->user->nikeys);
> > > > -                   spin_unlock(&key_user_lock);
> > > >                     mark_key_instantiated(key, 0);
> > > > +                   spin_unlock(&key_user_lock);
> > > >                     notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
> > > >
> > > >                     if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> > > > @@ -613,8 +613,8 @@ int key_reject_and_link(struct key *key,
> > > >             /* mark the key as being negatively instantiated */
> > > >             spin_lock(&key_user_lock);
> > > >             atomic_inc(&key->user->nikeys);
> > > > -           spin_unlock(&key_user_lock);
> > > >             mark_key_instantiated(key, -error);
> > > > +           spin_unlock(&key_user_lock);
> > > >             notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
> > > >             key_set_expiry(key, ktime_get_real_seconds() + timeout);
> > > >
> > > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > > > index c17924609317..83a9575b084e 100644
> > > > --- a/security/keys/keyctl.c
> > > > +++ b/security/keys/keyctl.c
> > > > @@ -1004,6 +1004,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > > >             if (!newowner)
> > > >                     goto error_put;
> > > >
> > > > +           spin_lock(&key_user_lock);
> > > > +
> > > >             /* transfer the quota burden to the new user */
> > > >             if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
> > > >                     unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
> > > > @@ -1036,11 +1038,10 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > > >                     atomic_inc(&newowner->nikeys);
> > > >             }
> > > >
> > > > -           spin_lock(&key_user_lock);
> > > >             zapowner = key->user;
> > > >             key->user = newowner;
> > > > -           spin_unlock(&key_user_lock);
> > > >             key->uid = uid;
> > > > +           spin_unlock(&key_user_lock);
> > > >     }
> > > >
> > > >     /* change the GID */
> > > > @@ -1060,6 +1061,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
> > > >
> > > >  quota_overrun:
> > > >     spin_unlock_irqrestore(&newowner->lock, flags);
> > > > +   spin_unlock(&key_user_lock);
> > > >     zapowner = newowner;
> > > >     ret = -EDQUOT;
> > > >     goto error_put;
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > I double-checked this patch too given the concerns on 1/2 but nope, this
> > > does not raise similar concerns as every critical section is strictly
> > > related to ownership change.
> > >
> > > E.g., I can be sure that the granularity is where it should be and locks
> > > are actually needed in the first place.
> > >
> > > Thus, I'm still including this patch to my next PR, and drop the first
> > > one.
> >
> > By dropping 1/2 I found out that 2/2 key.c becomes:
> >
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index a438c4508595..de63120c51ad 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -449,6 +449,7 @@ static int __key_instantiate_and_link(struct key *key,
> >                         /* mark the key as being instantiated */
> >                         atomic_inc(&key->user->nikeys);
> >                         mark_key_instantiated(key, 0);
> > +                       spin_unlock(&key_user_lock);
> >                         notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
> >
> >                         if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
> > @@ -606,6 +607,7 @@ int key_reject_and_link(struct key *key,
> >                 /* mark the key as being negatively instantiated */
> >                 atomic_inc(&key->user->nikeys);
> >                 mark_key_instantiated(key, -error);
> > +               spin_unlock(&key_user_lock);
> >                 notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
> >                 key_set_expiry(key, ktime_get_real_seconds() + timeout);
> >
> > This type of interleaving should never happen in a patch series.
> >
> > Please don't rush your changes like this in future.
> >
> > Br, Jarkko
> 
> Sorry for the mistake, I should have carefully verified that the tree
> remained valid at each step of the series.
> 
> I will rework the locking on 1/2 to inspect if there might be any
> unnecessary coverage, and verify each intermediate commit
> independently before sending a revised version for the patch series.

No problem. I just wanted to underline this :-) And I will gladly take
part of the blame given that I also rushed and this should have been
catched and fixed during the review cycle.

So let's just address this, phase down a bit so that at least correct
stuff gets out (and as many review cycles that requires).

The basis of the fix is not rotten. It does address the correct
root cause and is going into right direction. It's just not taken
to the end.

> 
> Best regards,
> Chengfeng

Br, Jarkko