From: Jann Horn jannh@google.com
mainline inclusion from mainline-v5.2-rc1 commit 0b9dc6c9f01c4a726558b82a3b6082a89d264eb5 category: bugfix bugzilla: https://gitee.com/openeuler/kernel/issues/I9355O CVE: NA
Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
--------------------------------
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 Conflicts: security/keys/process_keys.c security/keys/request_key.c Signed-off-by: GUO Zihua guozihua@huawei.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(-)
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h index 02c3b646bd07..38d3606ff09b 100644 --- a/include/linux/sched/user.h +++ b/include/linux/sched/user.h @@ -32,6 +32,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 diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index d776d2114651..b5a8202af128 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -59,7 +59,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; } @@ -112,8 +112,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;
@@ -391,10 +394,10 @@ 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) { 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();
@@ -414,9 +417,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 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags, 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) @@ -618,7 +621,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags, 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; @@ -630,7 +633,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags, 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; diff --git a/security/keys/request_key.c b/security/keys/request_key.c index 1a547cec8385..f62bb12bca41 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -294,11 +294,12 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
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: