From 0b9dc6c9f01c4a726558b82a3b6082a89d264eb5 Mon Sep 17 00:00:00 2001
From: Jann Horn <jannh@google.com>
Date: Wed, 27 Mar 2019 16:55:08 +0100
Subject: [PATCH] keys: safe concurrent user->{session,uid}_keyring access
Git-commit: 0b9dc6c9f01c4a726558b82a3b6082a89d264eb5
Patch-mainline: v5.2-rc1
References: bsc#1135642
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: 69664cf16af4 ("keys: don't generate user and user session keyrings unless they're accessed")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: James Morris <james.morris@microsoft.com>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
include/linux/sched/user.h | 7 +++++++
security/keys/process_keys.c | 31 +++++++++++++++++--------------
security/keys/request_key.c | 5 +++--
3 files changed, 27 insertions(+), 16 deletions(-)
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -28,6 +28,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
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -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);
@@ -339,6 +341,7 @@ void key_fsgid_changed(struct task_struc
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;
@@ -352,9 +355,9 @@ key_ref_t search_my_process_keyrings(str
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;
@@ -370,9 +373,9 @@ key_ref_t search_my_process_keyrings(str
}
/* 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;
@@ -390,10 +393,10 @@ key_ref_t search_my_process_keyrings(str
}
/* search the session keyring */
- if (ctx->cred->session_keyring) {
+ if (cred->session_keyring) {
rcu_read_lock();
key_ref = keyring_search_aux(
- make_key_ref(rcu_dereference(ctx->cred->session_keyring), 1),
+ make_key_ref(rcu_dereference(cred->session_keyring), 1),
ctx);
rcu_read_unlock();
@@ -413,9 +416,9 @@ key_ref_t search_my_process_keyrings(str
}
}
/* 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;
@@ -601,7 +604,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)
@@ -617,7 +620,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;
@@ -629,7 +632,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;
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -308,11 +308,12 @@ static int construct_get_dest_keyring(st
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: