Commit 0b9dc6c9 authored by Jann Horn's avatar Jann Horn Committed by James Morris
Browse files

keys: safe concurrent user->{session,uid}_keyring access 



The current code can perform concurrent updates and reads on
user->session_keyring and user->uid_keyring. Add a comment to
struct user_struct to document the nontrivial locking semantics, and use
READ_ONCE() for unlocked readers and smp_store_release() for writers to
prevent memory ordering issues.

Fixes: 69664cf1 ("keys: don't generate user and user session keyrings unless they're accessed")
Signed-off-by: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarJames Morris <james.morris@microsoft.com>
parent 5c7e372c

include/linux/sched/user.h

+7 −0
Original line number Diff line number Diff line
@@ -31,6 +31,13 @@ struct user_struct { 
	atomic_long_t pipe_bufs;  /* how many pages are allocated in pipe buffers */



#ifdef CONFIG_KEYS

	/*

	 * These pointers can only change from NULL to a non-NULL value once.

	 * Writes are protected by key_user_keyring_mutex.

	 * Unlocked readers should use READ_ONCE() unless they know that

	 * install_user_keyrings() has been called successfully (which sets

	 * these members to non-NULL values, preventing further modifications).

	 */

	struct key *uid_keyring;	/* UID specific keyring */

	struct key *session_keyring;	/* UID's default session keyring */

#endif

security/keys/process_keys.c

+17 −14
Original line number Diff line number Diff line
@@ -58,7 +58,7 @@ int install_user_keyrings(void) 


	kenter("%p{%u}", user, uid);



	if (user->uid_keyring && user->session_keyring) {

	if (READ_ONCE(user->uid_keyring) && READ_ONCE(user->session_keyring)) {

		kleave(" = 0 [exist]");

		return 0;

	}
@@ -111,8 +111,10 @@ int install_user_keyrings(void) 
		}



		/* install the keyrings */

		user->uid_keyring = uid_keyring;

		user->session_keyring = session_keyring;

		/* paired with READ_ONCE() */

		smp_store_release(&user->uid_keyring, uid_keyring);

		/* paired with READ_ONCE() */

		smp_store_release(&user->session_keyring, session_keyring);

	}



	mutex_unlock(&key_user_keyring_mutex);
@@ -340,6 +342,7 @@ void key_fsgid_changed(struct task_struct *tsk) 
key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)

{

	key_ref_t key_ref, ret, err;

	const struct cred *cred = ctx->cred;



	/* we want to return -EAGAIN or -ENOKEY if any of the keyrings were

	 * searchable, but we failed to find a key or we found a negative key;
@@ -353,9 +356,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx) 
	err = ERR_PTR(-EAGAIN);



	/* search the thread keyring first */

	if (ctx->cred->thread_keyring) {

	if (cred->thread_keyring) {

		key_ref = keyring_search_aux(

			make_key_ref(ctx->cred->thread_keyring, 1), ctx);

			make_key_ref(cred->thread_keyring, 1), ctx);

		if (!IS_ERR(key_ref))

			goto found;
@@ -371,9 +374,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx) 
	}



	/* search the process keyring second */

	if (ctx->cred->process_keyring) {

	if (cred->process_keyring) {

		key_ref = keyring_search_aux(

			make_key_ref(ctx->cred->process_keyring, 1), ctx);

			make_key_ref(cred->process_keyring, 1), ctx);

		if (!IS_ERR(key_ref))

			goto found;
@@ -392,9 +395,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx) 
	}



	/* search the session keyring */

	if (ctx->cred->session_keyring) {

	if (cred->session_keyring) {

		key_ref = keyring_search_aux(

			make_key_ref(ctx->cred->session_keyring, 1), ctx);

			make_key_ref(cred->session_keyring, 1), ctx);



		if (!IS_ERR(key_ref))

			goto found;
@@ -413,9 +416,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx) 
		}

	}

	/* or search the user-session keyring */

	else if (ctx->cred->user->session_keyring) {

	else if (READ_ONCE(cred->user->session_keyring)) {

		key_ref = keyring_search_aux(

			make_key_ref(ctx->cred->user->session_keyring, 1),

			make_key_ref(READ_ONCE(cred->user->session_keyring), 1),

			ctx);

		if (!IS_ERR(key_ref))

			goto found;
@@ -602,7 +605,7 @@ try_again: 
				goto error;

			goto reget_creds;

		} else if (ctx.cred->session_keyring ==

			   ctx.cred->user->session_keyring &&

			   READ_ONCE(ctx.cred->user->session_keyring) &&

			   lflags & KEY_LOOKUP_CREATE) {

			ret = join_session_keyring(NULL);

			if (ret < 0)
@@ -616,7 +619,7 @@ try_again: 
		break;



	case KEY_SPEC_USER_KEYRING:

		if (!ctx.cred->user->uid_keyring) {

		if (!READ_ONCE(ctx.cred->user->uid_keyring)) {

			ret = install_user_keyrings();

			if (ret < 0)

				goto error;
@@ -628,7 +631,7 @@ try_again: 
		break;



	case KEY_SPEC_USER_SESSION_KEYRING:

		if (!ctx.cred->user->session_keyring) {

		if (!READ_ONCE(ctx.cred->user->session_keyring)) {

			ret = install_user_keyrings();

			if (ret < 0)

				goto error;

security/keys/request_key.c

+3 −2
Original line number Diff line number Diff line
@@ -293,11 +293,12 @@ static int construct_get_dest_keyring(struct key **_dest_keyring) 
			/* fall through */

		case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:

			dest_keyring =

				key_get(cred->user->session_keyring);

				key_get(READ_ONCE(cred->user->session_keyring));

			break;



		case KEY_REQKEY_DEFL_USER_KEYRING:

			dest_keyring = key_get(cred->user->uid_keyring);

			dest_keyring =

				key_get(READ_ONCE(cred->user->uid_keyring));

			break;



		case KEY_REQKEY_DEFL_GROUP_KEYRING:

CRA Git | Maintained and supported by SUSTech CRA and CCSE