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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] keys: NULL dereference in keyctl_session_to_parent() without session keyring
--

commit e79a6a32e7b5e470d5cab49936bcd7b8bfc551d1
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Return user session keyring on lookup

This commit changes lookup_user_key() so that looking up
KEY_SPEC_SESSION_KEYRING without KEY_LOOKUP_CREATE returns the user session
keyring directly instead of installing it into the current task's
credentials.

> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index a63c46bb2d148..a5aa056a67259 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
[ ... ]
> @@ -665,20 +640,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;

[Severity: Critical]
Could returning user_session here without installing it into the current
task's credentials cause a NULL pointer dereference in
keyctl_session_to_parent()?

In security/keys/keyctl.c, keyctl_session_to_parent() looks up
KEY_SPEC_SESSION_KEYRING without KEY_LOOKUP_CREATE and stores the returned
keyring in cred->session_keyring:

security/keys/keyctl.c:keyctl_session_to_parent() {
	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_LINK);
	if (IS_ERR(keyring_r))
		return PTR_ERR(keyring_r);
	...
	cred->session_keyring = key_ref_to_ptr(keyring_r);
	...
	mycred = current_cred();
	pcred = __task_cred(parent);
	if (mycred == pcred ||
	    mycred->session_keyring == pcred->session_keyring) {
		ret = 0;
		goto unlock;
	}
	...
	/* 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))
		goto unlock;
}

Previously, lookup_user_key() installed user_session into the current
task's credentials, guaranteeing that mycred->session_keyring was non-NULL.

With this change, if a child process has no session keyring
(mycred->session_keyring is NULL) and its parent has a non-NULL
pcred->session_keyring with matching ownership, the check
mycred->session_keyring == pcred->session_keyring is false, and
!uid_eq(mycred->session_keyring->uid, mycred->euid) dereferences NULL
while holding write_lock_irq(&tasklist_lock) and rcu_read_lock().

Additionally, if both the child and the parent have session_keyring == NULL,
mycred->session_keyring == pcred->session_keyring evaluates to true (NULL ==
NULL), causing keyctl_session_to_parent() to return 0 early without
installing cred->session_keyring onto the parent.

Should keyctl_session_to_parent() check cred->session_keyring instead of
mycred->session_keyring (and avoid the mycred == pcred early exit when
mycred->session_keyring is NULL)?

>  		} else if (test_bit(KEY_FLAG_UID_KEYRING,
>  				    &ctx.cred->session_keyring->flags) &&
>  			   lflags & KEY_LOOKUP_CREATE) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019145624.309790-1-jarkko@kernel.org?part=1