On Wed, Apr 24, 2019 at 6:14 PM David Howells <dhowells@xxxxxxxxxx> wrote:
> Move the user and user-session keyrings to the user_namespace struct rather
> than pinning them from the user_struct struct. This prevents these
> keyrings from propagating across user-namespaces boundaries with regard to
> the KEY_SPEC_* flags, thereby making them more useful in a containerised
> environment.
>
> The issue is that a single user_struct may be represent UIDs in several
> different namespaces.
>
> The way the patch does this is by attaching a 'register keyring' in each
> user_namespace and then sticking the user and user-session keyrings into
> that. It can then be searched to retrieve them.
Overall, this looks good to me, apart from some details.
The user_keyring_register keyring is basically just used like an
xarray/idr/... that maps from namespaced UIDs to keyrings, right? (Not
saying it's a bad idea, just want to make sure I understand it
correctly.)
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 90457015fa3f..44a5a4a8a269 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -67,12 +67,13 @@ struct user_namespace {
> #ifdef CONFIG_KEYS
> /* List of joinable keyrings in this namespace */
> struct list_head keyring_name_list;
> + struct key *user_keyring_register;
Maybe a comment about locking semantics above user_keyring_register?
"Only written once, may be read locklessly with READ_ONCE()", or
something like that?
> + struct rw_semaphore keyring_sem;
> #endif
[...]
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 70fa20888ca8..ff62d90415ae 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -8,7 +8,7 @@
> * as published by the Free Software Foundation; either version
> * 2 of the License, or (at your option) any later version.
> */
> -
> +#define __KDEBUG
Was that supposed to be in here, or did you commit that accidentally?
[...]
> @@ -39,98 +37,173 @@ struct key_user root_key_user = {
> };
>
> /*
> - * Install the user and user session keyrings for the current process's UID.
> + * Get or create a user register keyring.
> */
> -int install_user_keyrings(void)
> +static struct key *get_user_register(struct user_namespace *user_ns)
> {
> - struct user_struct *user;
> - const struct cred *cred;
> - struct key *uid_keyring, *session_keyring;
> + struct key *reg_keyring = user_ns->user_keyring_register;
This is a lockless read of a field that may be written concurrently;
this should be READ_ONCE(). (Especially on alpha, I think the memory
ordering will actually be incorrect without READ_ONCE().)
> + if (reg_keyring)
> + return reg_keyring;
> +
> + down_write(&user_ns->keyring_sem);
> +
> + /* Make sure there's a register keyring. It gets owned by the
> + * user_namespace's owner.
> + */
> + reg_keyring = user_ns->user_keyring_register;
> + if (!reg_keyring) {
> + reg_keyring = keyring_alloc(".user_reg", user_ns->owner, INVALID_GID,
> + &init_cred,
> + KEY_POS_WRITE | KEY_POS_SEARCH |
> + KEY_USR_VIEW | KEY_USR_READ,
> + 0,
> + NULL, NULL);
> + if (!IS_ERR(reg_keyring))
> + user_ns->user_keyring_register = reg_keyring;
This is a write of a pointer that may be read concurrently; this
should be smp_store_release().
> + }
> +
> + up_write(&user_ns->keyring_sem);
> +
> + /* We don't return a ref since the keyring is pinned by the user_ns */
> + return reg_keyring;
> +}
> +
> +/*
> + * Look up the user and user session keyrings for the current process's UID,
> + * creating them if they don't exist.
> + */
> +int look_up_user_keyrings(struct key **_user_keyring,
> + struct key **_user_session_keyring)
> +{
> + const struct cred *cred = current_cred();
> + struct user_namespace *user_ns = current_user_ns();
> + struct key *reg_keyring, *uid_keyring, *session_keyring;
> key_perm_t user_keyring_perm;
> + key_ref_t uid_keyring_r, session_keyring_r;
> + uid_t uid = from_kuid(user_ns, cred->user->uid);
> char buf[20];
> int ret;
> - uid_t uid;
>
> user_keyring_perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL;
> - cred = current_cred();
> - user = cred->user;
> - uid = from_kuid(cred->user_ns, user->uid);
>
> - kenter("%p{%u}", user, uid);
> + kenter("%u", uid);
>
> - if (READ_ONCE(user->uid_keyring) && READ_ONCE(user->session_keyring)) {
> - kleave(" = 0 [exist]");
> - return 0;
> - }
> + reg_keyring = get_user_register(user_ns);
> + if (IS_ERR(reg_keyring))
> + return PTR_ERR(reg_keyring);
>
> - mutex_lock(&key_user_keyring_mutex);
> + down_write(&user_ns->keyring_sem);
> ret = 0;
>
> - if (!user->uid_keyring) {
> - /* get the UID-specific keyring
> - * - there may be one in existence already as it may have been
> - * pinned by a session, but the user_struct pointing to it
> - * may have been destroyed by setuid */
> - sprintf(buf, "_uid.%u", uid);
> -
> - uid_keyring = find_keyring_by_name(buf, true);
> + /* Get the user keyring. Note that there may be one in existence
> + * already as it may have been pinned by a session, but the user_struct
> + * pointing to it may have been destroyed by setuid.
> + */
> + sprintf(buf, "_uid.%u", uid);
I know that the sprintf() calls here and below can't overrun the
buffer, but it'd be nice if you could use "snprintf(buf, sizeof(buf),
...)" anyway.
[...]
> }
>
> +/*
> + * Get the user session keyring if it exists, but don't create it if it
> + * doesn't.
> + */
> +struct key *get_user_session_keyring(void)
> +{
> + const struct cred *cred = current_cred();
> + struct user_namespace *user_ns = current_user_ns();
> + struct key *reg_keyring = user_ns->user_keyring_register;
(READ_ONCE() again)
> + key_ref_t session_keyring_r;
> + char buf[20];
> +
> + if (!reg_keyring)
> + return NULL;
> +
> + sprintf(buf, "_uid_ses.%u", from_kuid(user_ns, cred->user->uid));
> +
> + session_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
> + &key_type_keyring, buf, false);
> + if (IS_ERR(session_keyring_r))
> + return NULL;
> + return key_ref_to_ptr(session_keyring_r);
> +}
[...]
> @@ -416,10 +490,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
> }
> }
> /* or search the user-session keyring */
> - else if (READ_ONCE(cred->user->session_keyring)) {
> - key_ref = keyring_search_aux(
> - make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
> - ctx);
> + else if ((user_session = get_user_session_keyring())) {
> + key_ref = keyring_search_aux(make_key_ref(user_session, 1),
> + ctx);
> if (!IS_ERR(key_ref))
> goto found;
I'm not sure I understand this code. In the "goto found" case, the
key_put() below is skipped, right? Is that intentional?
>
> @@ -435,6 +508,8 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
> err = key_ref;
> break;
> }
> +
> + key_put(user_session);
> }
>
> /* no key - decide on the error we're going to go for */
[...]
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 39802540ffff..d95627888f5a 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -96,7 +96,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
> struct request_key_auth *rka = get_request_key_auth(authkey);
> const struct cred *cred = current_cred();
> key_serial_t prkey, sskey;
> - struct key *key = rka->target_key, *keyring, *session;
> + struct key *key = rka->target_key, *keyring, *session, *user_session;
> char *argv[9], *envp[3], uid_str[12], gid_str[12];
> char key_str[12], keyring_str[3][12];
> char desc[20];
> @@ -104,9 +104,9 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
> kenter("{%d},{%d},%s", key->serial, authkey->serial, rka->op);
>
> - ret = install_user_keyrings();
> + ret = look_up_user_keyrings(NULL, &user_session);
> if (ret < 0)
> - goto error_alloc;
> + goto error_us;
>
> /* allocate a new session keyring */
> sprintf(desc, "_req.%u", key->serial);
> @@ -144,7 +144,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
> session = cred->session_keyring;
> if (!session)
> - session = cred->user->session_keyring;
> + session = user_session;
> sskey = session->serial;
>
> sprintf(keyring_str[2], "%d", sskey);
> @@ -187,6 +187,8 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
> error_alloc:
> complete_request_key(authkey, ret);
> +error_us:
> + key_put(user_session);
> kleave(" = %d", ret);
> return ret;
> }
This looks weird. If the look_up_user_keyrings() fails, user_session
might still be an uninitialized pointer, right? And then the "goto
error_us" jumps down here and calls key_put() on that?
> @@ -289,16 +291,19 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
>
> if (dest_keyring)
> break;
> + /* Fall through */
>
> /* fall through */
> case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:
Why two "fall through" comments?
