Re: [PATCH v3 1/2] KEYS: Return user session keyring on lookup

[Jarkko Sakkinen <jarkko@kernel.org>]

On Fri, Nov 06, 2026 at 12:00:49AM +0200, Jarkko Sakkinen wrote:
> A process without a session keyring looking up KEY_SPEC_SESSION_KEYRING
> without KEY_LOOKUP_CREATE mutates the credentials. This causes struct creds
> instances shared with other subsystems to become stale.
> 
> Address this by returning the resolved user session keyring directly
> without installing it into credentials.
> 
> Cc: keyrings@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: stable@vger.kernel.org # v5.10+
> Fixes: d84f4f992cbd ("CRED: Inaugurate COW credentials")
> Suggested-by: Jann Horn <jannh@google.com>
> Closes: https://lore.kernel.org/keyrings/CAG48ez0XjVSR=M--UBauTm8sJuc+ZbhKzaPa3a6wrSVHyoKevw@mail.gmail.com/
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

These patches have zero Sashiko remarks and are looking pretty good
overall.

Is there any reason not to apply them?

> ---
> v3:
> In keyctl_session_to_parent(), check the resolved cred->session_keyring
> instead of mycred->session_keyring.
> v2:
> - Added mandatory tags.
> - No code changes.
> - Link: https://lore.kernel.org/keyrings/20261019145624.309790-1-jarkko@kernel.org/
>  Documentation/security/keys/core.rst |  6 ++--
>  security/keys/keyctl.c               |  5 ++--
>  security/keys/process_keys.c         | 44 +++++++---------------------
>  3 files changed, 16 insertions(+), 39 deletions(-)
> ---
>  Documentation/security/keys/core.rst |  6 ++--
>  security/keys/keyctl.c               |  5 ++--
>  security/keys/process_keys.c         | 44 +++++++---------------------
>  3 files changed, 16 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 37ed42382f16..e3a215e55f81 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -165,8 +165,10 @@ The key service provides a number of features besides keys:
>       When a process changes its real UID, if it used to have no session key, it
>       will be subscribed to the default session key for the new UID.
>  
> -     If a process attempts to access its session key when it doesn't have one,
> -     it will be subscribed to the default for its current UID.
> +     If a process attempts to access its session keyring when it doesn't have
> +     one, the default user session keyring for its current UID is returned
> +     without being installed into its credentials.  If creation is requested,
> +     an anonymous session keyring is installed.
>  
>    *  Each user has two quotas against which the keys they own are tracked. One
>       limits the total number of keys and keyrings, the other limits the total
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d6b10fd67627..b11716f88ded 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1682,8 +1682,7 @@ long keyctl_session_to_parent(void)
>  	 * there's no point */
>  	mycred = current_cred();
>  	pcred = __task_cred(parent);
> -	if (mycred == pcred ||
> -	    mycred->session_keyring == pcred->session_keyring) {
> +	if (cred->session_keyring == pcred->session_keyring) {
>  		ret = 0;
>  		goto unlock;
>  	}
> @@ -1701,7 +1700,7 @@ long keyctl_session_to_parent(void)
>  	/* the keyrings must have the same UID */
>  	if ((pcred->session_keyring &&
>  	     !uid_eq(pcred->session_keyring->uid, mycred->euid)) ||
> -	    !uid_eq(mycred->session_keyring->uid, mycred->euid))
> +	    !uid_eq(cred->session_keyring->uid, mycred->euid))
>  		goto unlock;
>  
>  	/* cancel an already pending keyring replacement */
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 3af71c6322af..cdf03b8124d0 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -356,31 +356,6 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring)
>  	return 0;
>  }
>  
> -/*
> - * Install the given keyring as the session keyring of the current task,
> - * replacing the existing one if any.  If the given keyring is NULL, then
> - * install a new anonymous session keyring.
> - *
> - * Return: 0 on success; -errno on failure.
> - */
> -static int install_session_keyring(struct key *keyring)
> -{
> -	struct cred *new;
> -	int ret;
> -
> -	new = prepare_creds();
> -	if (!new)
> -		return -ENOMEM;
> -
> -	ret = install_session_keyring_to_cred(new, keyring);
> -	if (ret < 0) {
> -		abort_creds(new);
> -		return ret;
> -	}
> -
> -	return commit_creds(new);
> -}
> -
>  /*
>   * Handle the fsuid changing.
>   */
> @@ -720,20 +695,21 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
>  
>  	case KEY_SPEC_SESSION_KEYRING:
>  		if (!ctx.cred->session_keyring) {
> -			/* always install a session keyring upon access if one
> -			 * doesn't exist yet */
>  			ret = look_up_user_keyrings(NULL, &user_session);
>  			if (ret < 0)
>  				goto error;
> -			if (lflags & KEY_LOOKUP_CREATE)
> +
> +			if (lflags & KEY_LOOKUP_CREATE) {
> +				key_put(user_session);
>  				ret = join_session_keyring(NULL);
> -			else
> -				ret = install_session_keyring(user_session);
> +				if (ret < 0)
> +					goto error;
> +				goto reget_creds;
> +			}
>  
> -			key_put(user_session);
> -			if (ret < 0)
> -				goto error;
> -			goto reget_creds;
> +			key = user_session;
> +			key_ref = make_key_ref(key, 1);
> +			break;
>  		} else if (test_bit(KEY_FLAG_UID_KEYRING,
>  				    &ctx.cred->session_keyring->flags) &&
>  			   lflags & KEY_LOOKUP_CREATE) {
> -- 
> 2.47.3
> 


Br, Jarkko