[PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Cen Zhang]

Named keyring lookup must keep the storage containing the owner UID
alive through the namespace mapping check.  find_keyring_by_name()
reads keyring->user->uid under keyring_name_lock, but that lock protects
the keyring's name entry and allocation, not its separate key_user.

A named session-keyring join can overlap a privileged KEYCTL_CHOWN on
another CPU.  If the keyring holds the last reference to its old dynamic
key_user and the ownership transfer succeeds, the following ordering is
possible:

    Named join                         Chown
    find_keyring_by_name()              keyctl_chown_key()
      read_lock(keyring_name_lock)       down_write(key->sem)
      load old keyring->user
                                         replace key->user and key->uid
                                         up_write(key->sem)
                                         key_put(key)
                                         key_user_put(old user): free
      read old user->uid
      read_unlock(keyring_name_lock)

Chown neither takes keyring_name_lock nor key_session_mutex, so it can
free the old account between the pointer load and the UID read.  The
lookup then reads freed memory even though the keyring itself is alive.

Use keyring->uid for the mapping check.  key_alloc() initializes this
inline owner UID and keyctl_chown_key() updates it on chown.
The existing name lock keeps its containing keyring allocated throughout
the check, so lookup no longer depends on the account's lifetime.  This
also matches the owner UID used by the permission check.

KASAN report as below:

    BUG: KASAN: slab-use-after-free in find_keyring_by_name+0x577/0x5d0
    Read of size 4 at addr ffff8881128ae2ec by task keycase/500

Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
Assisted-by: LLM
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Changes in v2:
- Trim the KASAN report to its two-line fault summary.

Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0002-v2-dd6c20a1d92928b33ab7@gmail.com

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 15bf4af8f28218ec3f12c97630d1c76939af7eca..46f774be72967a9bd16b0ddfdf323eda36edeff4 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1158,7 +1158,7 @@ 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))
+		if (!kuid_has_mapping(ns, keyring->uid))
 			continue;
 
 		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[sashiko-bot]

> Named keyring lookup must keep the storage containing the owner UID
> alive through the namespace mapping check.  find_keyring_by_name()
> reads keyring->user->uid under keyring_name_lock, but that lock protects
> the keyring's name entry and allocation, not its separate key_user.
> 
> A named session-keyring join can overlap a privileged KEYCTL_CHOWN on
> another CPU.  If the keyring holds the last reference to its old dynamic
> key_user and the ownership transfer succeeds, the following ordering is
> possible:
> 
>     Named join                         Chown
>     find_keyring_by_name()              keyctl_chown_key()
>       read_lock(keyring_name_lock)       down_write(key->sem)
>       load old keyring->user
>                                          replace key->user and key->uid
> [ ... ]
>     Read of size 4 at addr ffff8881128ae2ec by task keycase/500
> 
> Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/pm-key-management-objects-candidate-0002-v3-2baa8cad2bb554364e10@gmail.com?part=1

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Jarkko Sakkinen]

On Fri, Oct 09, 2026 at 01:11:59PM +0800, Cen Zhang wrote:
> Named keyring lookup must keep the storage containing the owner UID
> alive through the namespace mapping check.  find_keyring_by_name()
> reads keyring->user->uid under keyring_name_lock, but that lock protects
> the keyring's name entry and allocation, not its separate key_user.
> 
> A named session-keyring join can overlap a privileged KEYCTL_CHOWN on
> another CPU.  If the keyring holds the last reference to its old dynamic
> key_user and the ownership transfer succeeds, the following ordering is
> possible:
> 
>     Named join                         Chown
>     find_keyring_by_name()              keyctl_chown_key()
>       read_lock(keyring_name_lock)       down_write(key->sem)
>       load old keyring->user
>                                          replace key->user and key->uid
>                                          up_write(key->sem)
>                                          key_put(key)
>                                          key_user_put(old user): free
>       read old user->uid
>       read_unlock(keyring_name_lock)
> 
> Chown neither takes keyring_name_lock nor key_session_mutex, so it can
> free the old account between the pointer load and the UID read.  The
> lookup then reads freed memory even though the keyring itself is alive.
> 
> Use keyring->uid for the mapping check.  key_alloc() initializes this
> inline owner UID and keyctl_chown_key() updates it on chown.
> The existing name lock keeps its containing keyring allocated throughout
> the check, so lookup no longer depends on the account's lifetime.  This
> also matches the owner UID used by the permission check.
> 
> KASAN report as below:
> 
>     BUG: KASAN: slab-use-after-free in find_keyring_by_name+0x577/0x5d0
>     Read of size 4 at addr ffff8881128ae2ec by task keycase/500
> 
> Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Trim the KASAN report to its two-line fault summary.
> 
> Link to v1: https://lore.kernel.org/r/pm-key-management-objects-candidate-0002-v2-dd6c20a1d92928b33ab7@gmail.com
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 15bf4af8f28218ec3f12c97630d1c76939af7eca..46f774be72967a9bd16b0ddfdf323eda36edeff4 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1158,7 +1158,7 @@ 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))
> +		if (!kuid_has_mapping(ns, keyring->uid))
>  			continue;
>  
>  		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))

Thanks.


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

Br, Jarkko

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Eric Biggers]

On Fri, Oct 09, 2026 at 01:11:59PM +0800, Cen Zhang wrote:
> Named keyring lookup must keep the storage containing the owner UID
> alive through the namespace mapping check.  find_keyring_by_name()
> reads keyring->user->uid under keyring_name_lock, but that lock protects
> the keyring's name entry and allocation, not its separate key_user.
> 
> A named session-keyring join can overlap a privileged KEYCTL_CHOWN on
> another CPU.  If the keyring holds the last reference to its old dynamic
> key_user and the ownership transfer succeeds, the following ordering is
> possible:
> 
>     Named join                         Chown
>     find_keyring_by_name()              keyctl_chown_key()
>       read_lock(keyring_name_lock)       down_write(key->sem)
>       load old keyring->user
>                                          replace key->user and key->uid
>                                          up_write(key->sem)
>                                          key_put(key)
>                                          key_user_put(old user): free
>       read old user->uid
>       read_unlock(keyring_name_lock)
> 
> Chown neither takes keyring_name_lock nor key_session_mutex, so it can
> free the old account between the pointer load and the UID read.  The
> lookup then reads freed memory even though the keyring itself is alive.
> 
> Use keyring->uid for the mapping check.  key_alloc() initializes this
> inline owner UID and keyctl_chown_key() updates it on chown.
> The existing name lock keeps its containing keyring allocated throughout
> the check, so lookup no longer depends on the account's lifetime.  This
> also matches the owner UID used by the permission check.
> 
> KASAN report as below:
> 
>     BUG: KASAN: slab-use-after-free in find_keyring_by_name+0x577/0x5d0
>     Read of size 4 at addr ffff8881128ae2ec by task keycase/500
> 
> Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

Don't key_serial_next() and find_ge_key() (used when reading from
/proc/keys) have the same bug?

- Eric

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Cen Zhang]

Hi Eric,

Eric Biggers <ebiggers@kernel.org> 于2026年10月17日周六 11:51写道：
>
> Don't key_serial_next() and find_ge_key() (used when reading from
> /proc/keys) have the same bug?
>
> - Eric

Thansk a lot for the reivew. You're right — key_serial_next() and
find_ge_key() have
the same issue.

I'll send a follow-up patch to fix those as well.

Thanks for the catch again.

Best regards,
Cen Zhang

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Jarkko Sakkinen]

On Fri, Oct 16, 2026 at 08:51:56PM -0700, Eric Biggers wrote:
> On Fri, Oct 09, 2026 at 01:11:59PM +0800, Cen Zhang wrote:
> > Named keyring lookup must keep the storage containing the owner UID
> > alive through the namespace mapping check.  find_keyring_by_name()
> > reads keyring->user->uid under keyring_name_lock, but that lock protects
> > the keyring's name entry and allocation, not its separate key_user.
> > 
> > A named session-keyring join can overlap a privileged KEYCTL_CHOWN on
> > another CPU.  If the keyring holds the last reference to its old dynamic
> > key_user and the ownership transfer succeeds, the following ordering is
> > possible:
> > 
> >     Named join                         Chown
> >     find_keyring_by_name()              keyctl_chown_key()
> >       read_lock(keyring_name_lock)       down_write(key->sem)
> >       load old keyring->user
> >                                          replace key->user and key->uid
> >                                          up_write(key->sem)
> >                                          key_put(key)
> >                                          key_user_put(old user): free
> >       read old user->uid
> >       read_unlock(keyring_name_lock)
> > 
> > Chown neither takes keyring_name_lock nor key_session_mutex, so it can
> > free the old account between the pointer load and the UID read.  The
> > lookup then reads freed memory even though the keyring itself is alive.
> > 
> > Use keyring->uid for the mapping check.  key_alloc() initializes this
> > inline owner UID and keyctl_chown_key() updates it on chown.
> > The existing name lock keeps its containing keyring allocated throughout
> > the check, so lookup no longer depends on the account's lifetime.  This
> > also matches the owner UID used by the permission check.
> > 
> > KASAN report as below:
> > 
> >     BUG: KASAN: slab-use-after-free in find_keyring_by_name+0x577/0x5d0
> >     Read of size 4 at addr ffff8881128ae2ec by task keycase/500
> > 
> > Fixes: 2ea190d0a006 ("keys: skip keys from another user namespace")
> > Assisted-by: LLM
> > Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> 
> Don't key_serial_next() and find_ge_key() (used when reading from
> /proc/keys) have the same bug?

With just a quick Saturday skim, yep, I think so.

> 
> - Eric

Br, Jarkko

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Jarkko Sakkinen]

On Sat, Oct 17, 2026 at 03:47:32PM +0800, Cen Zhang wrote:
> Hi Eric,
> 
> Eric Biggers <ebiggers@kernel.org> 于2026年10月17日周六 11:51写道：
> >
> > Don't key_serial_next() and find_ge_key() (used when reading from
> > /proc/keys) have the same bug?
> >
> > - Eric
> 
> Thansk a lot for the reivew. You're right — key_serial_next() and
> find_ge_key() have
> the same issue.
> 
> I'll send a follow-up patch to fix those as well.
> 
> Thanks for the catch again.
> 
> Best regards,
> Cen Zhang

Yeah, please do.

You put so much effort to scenario and you are nailing the bug
that way, and I do appreciate that, so here's what you should do
with the commit messages:

1. Do the first half like you always do. That's great. And I've
   also said that.
2. Think just a moment. If you get anxiety from that something
   that should be mentioned e.g., something that you did not get
   immediately yourself at first, write remarks those down. I'm
   100% sure you know when it might be good tip on something
   non-obvious.
3. Send that. If we feel it is lacking some info needed, let's
   iterate that. It's not a big deal.

I'm just underlining this because so little would have to be
improved to make this right :-)

Br, Jarkko

Re: [PATCH v2] keys: Avoid the owner account dereference in named keyring lookup

[Jarkko Sakkinen]

On Sat, Oct 17, 2026 at 07:28:31PM +0300, Jarkko Sakkinen wrote:
> On Sat, Oct 17, 2026 at 03:47:32PM +0800, Cen Zhang wrote:
> > Hi Eric,
> > 
> > Eric Biggers <ebiggers@kernel.org> 于2026年10月17日周六 11:51写道：
> > >
> > > Don't key_serial_next() and find_ge_key() (used when reading from
> > > /proc/keys) have the same bug?
> > >
> > > - Eric
> > 
> > Thansk a lot for the reivew. You're right — key_serial_next() and
> > find_ge_key() have
> > the same issue.
> > 
> > I'll send a follow-up patch to fix those as well.
> > 
> > Thanks for the catch again.
> > 
> > Best regards,
> > Cen Zhang
> 
> Yeah, please do.
> 
> You put so much effort to scenario and you are nailing the bug
> that way, and I do appreciate that, so here's what you should do
> with the commit messages:
> 
> 1. Do the first half like you always do. That's great. And I've
>    also said that.
> 2. Think just a moment. If you get anxiety from that something
>    that should be mentioned e.g., something that you did not get
>    immediately yourself at first, write remarks those down. I'm
>    100% sure you know when it might be good tip on something
>    non-obvious.
> 3. Send that. If we feel it is lacking some info needed, let's
>    iterate that. It's not a big deal.
> 
> I'm just underlining this because so little would have to be
> improved to make this right :-)

And also: I would not complain about anything if I did not want
you to send patches because it does not make sense to waste time
like that. So take it this from that angle. And they bring value
thanks to well described scenarios.

Br, Jarkko