--- /dev/null
+From 363b02dab09b3226f3bd1420dad9c72b79a42a76 Mon Sep 17 00:00:00 2001
+From: David Howells <dhowells@redhat.com>
+Date: Wed, 4 Oct 2017 16:43:25 +0100
+Subject: KEYS: Fix race between updating and finding a negative key
+
+From: David Howells <dhowells@redhat.com>
+
+commit 363b02dab09b3226f3bd1420dad9c72b79a42a76 upstream.
+
+Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
+error into one field such that:
+
+ (1) The instantiation state can be modified/read atomically.
+
+ (2) The error can be accessed atomically with the state.
+
+ (3) The error isn't stored unioned with the payload pointers.
+
+This deals with the problem that the state is spread over three different
+objects (two bits and a separate variable) and reading or updating them
+atomically isn't practical, given that not only can uninstantiated keys
+change into instantiated or rejected keys, but rejected keys can also turn
+into instantiated keys - and someone accessing the key might not be using
+any locking.
+
+The main side effect of this problem is that what was held in the payload
+may change, depending on the state. For instance, you might observe the
+key to be in the rejected state. You then read the cached error, but if
+the key semaphore wasn't locked, the key might've become instantiated
+between the two reads - and you might now have something in hand that isn't
+actually an error code.
+
+The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error
+code if the key is negatively instantiated. The key_is_instantiated()
+function is replaced with key_is_positive() to avoid confusion as negative
+keys are also 'instantiated'.
+
+Additionally, barriering is included:
+
+ (1) Order payload-set before state-set during instantiation.
+
+ (2) Order state-read before payload-read when using the key.
+
+Further separate barriering is necessary if RCU is being used to access the
+payload content after reading the payload pointers.
+
+Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
+Reported-by: Eric Biggers <ebiggers@google.com>
+Signed-off-by: David Howells <dhowells@redhat.com>
+Reviewed-by: Eric Biggers <ebiggers@google.com>
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+---
+ include/linux/key.h | 47 +++++++++++++++++++------------
+ net/dns_resolver/dns_key.c | 2 -
+ security/keys/big_key.c | 4 +-
+ security/keys/encrypted-keys/encrypted.c | 2 -
+ security/keys/gc.c | 8 ++---
+ security/keys/key.c | 31 +++++++++++++-------
+ security/keys/keyctl.c | 9 ++---
+ security/keys/keyring.c | 10 +++---
+ security/keys/proc.c | 7 +++-
+ security/keys/process_keys.c | 2 -
+ security/keys/request_key.c | 7 +---
+ security/keys/request_key_auth.c | 2 -
+ security/keys/trusted.c | 2 -
+ security/keys/user_defined.c | 4 +-
+ 14 files changed, 80 insertions(+), 57 deletions(-)
+
+--- a/include/linux/key.h
++++ b/include/linux/key.h
+@@ -126,6 +126,11 @@ static inline bool is_key_possessed(cons
+ return (unsigned long) key_ref & 1UL;
+ }
+
++enum key_state {
++ KEY_IS_UNINSTANTIATED,
++ KEY_IS_POSITIVE, /* Positively instantiated */
++};
++
+ /*****************************************************************************/
+ /*
+ * authentication token / access credential / keyring
+@@ -157,6 +162,7 @@ struct key {
+ * - may not match RCU dereferenced payload
+ * - payload should contain own length
+ */
++ short state; /* Key state (+) or rejection error (-) */
+
+ #ifdef KEY_DEBUGGING
+ unsigned magic;
+@@ -165,18 +171,16 @@ struct key {
+ #endif
+
+ unsigned long flags; /* status flags (change with bitops) */
+-#define KEY_FLAG_INSTANTIATED 0 /* set if key has been instantiated */
+-#define KEY_FLAG_DEAD 1 /* set if key type has been deleted */
+-#define KEY_FLAG_REVOKED 2 /* set if key had been revoked */
+-#define KEY_FLAG_IN_QUOTA 3 /* set if key consumes quota */
+-#define KEY_FLAG_USER_CONSTRUCT 4 /* set if key is being constructed in userspace */
+-#define KEY_FLAG_NEGATIVE 5 /* set if key is negative */
+-#define KEY_FLAG_ROOT_CAN_CLEAR 6 /* set if key can be cleared by root without permission */
+-#define KEY_FLAG_INVALIDATED 7 /* set if key has been invalidated */
+-#define KEY_FLAG_BUILTIN 8 /* set if key is built in to the kernel */
+-#define KEY_FLAG_ROOT_CAN_INVAL 9 /* set if key can be invalidated by root without permission */
+-#define KEY_FLAG_KEEP 10 /* set if key should not be removed */
+-#define KEY_FLAG_UID_KEYRING 11 /* set if key is a user or user session keyring */
++#define KEY_FLAG_DEAD 0 /* set if key type has been deleted */
++#define KEY_FLAG_REVOKED 1 /* set if key had been revoked */
++#define KEY_FLAG_IN_QUOTA 2 /* set if key consumes quota */
++#define KEY_FLAG_USER_CONSTRUCT 3 /* set if key is being constructed in userspace */
++#define KEY_FLAG_ROOT_CAN_CLEAR 4 /* set if key can be cleared by root without permission */
++#define KEY_FLAG_INVALIDATED 5 /* set if key has been invalidated */
++#define KEY_FLAG_BUILTIN 6 /* set if key is built in to the kernel */
++#define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */
++#define KEY_FLAG_KEEP 8 /* set if key should not be removed */
++#define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */
+
+ /* the key type and key description string
+ * - the desc is used to match a key against search criteria
+@@ -202,7 +206,6 @@ struct key {
+ struct list_head name_link;
+ struct assoc_array keys;
+ };
+- int reject_error;
+ };
+
+ /* This is set on a keyring to restrict the addition of a link to a key
+@@ -343,17 +346,27 @@ extern void key_set_timeout(struct key *
+ #define KEY_NEED_SETATTR 0x20 /* Require permission to change attributes */
+ #define KEY_NEED_ALL 0x3f /* All the above permissions */
+
++static inline short key_read_state(const struct key *key)
++{
++ /* Barrier versus mark_key_instantiated(). */
++ return smp_load_acquire(&key->state);
++}
++
+ /**
+- * key_is_instantiated - Determine if a key has been positively instantiated
++ * key_is_positive - Determine if a key has been positively instantiated
+ * @key: The key to check.
+ *
+ * Return true if the specified key has been positively instantiated, false
+ * otherwise.
+ */
+-static inline bool key_is_instantiated(const struct key *key)
++static inline bool key_is_positive(const struct key *key)
++{
++ return key_read_state(key) == KEY_IS_POSITIVE;
++}
++
++static inline bool key_is_negative(const struct key *key)
+ {
+- return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
+- !test_bit(KEY_FLAG_NEGATIVE, &key->flags);
++ return key_read_state(key) < 0;
+ }
+
+ #define rcu_dereference_key(KEY) \
+--- a/net/dns_resolver/dns_key.c
++++ b/net/dns_resolver/dns_key.c
+@@ -224,7 +224,7 @@ static int dns_resolver_match_preparse(s
+ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
+ {
+ seq_puts(m, key->description);
+- if (key_is_instantiated(key)) {
++ if (key_is_positive(key)) {
+ int err = PTR_ERR(key->payload.data[dns_key_error]);
+
+ if (err)
+--- a/security/keys/big_key.c
++++ b/security/keys/big_key.c
+@@ -245,7 +245,7 @@ void big_key_revoke(struct key *key)
+
+ /* clear the quota */
+ key_payload_reserve(key, 0);
+- if (key_is_instantiated(key) &&
++ if (key_is_positive(key) &&
+ (size_t)key->payload.data[big_key_len] > BIG_KEY_FILE_THRESHOLD)
+ vfs_truncate(path, 0);
+ }
+@@ -277,7 +277,7 @@ void big_key_describe(const struct key *
+
+ seq_puts(m, key->description);
+
+- if (key_is_instantiated(key))
++ if (key_is_positive(key))
+ seq_printf(m, ": %zu [%s]",
+ datalen,
+ datalen > BIG_KEY_FILE_THRESHOLD ? "file" : "buff");
+--- a/security/keys/encrypted-keys/encrypted.c
++++ b/security/keys/encrypted-keys/encrypted.c
+@@ -874,7 +874,7 @@ static int encrypted_update(struct key *
+ size_t datalen = prep->datalen;
+ int ret = 0;
+
+- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
++ if (key_is_negative(key))
+ return -ENOKEY;
+ if (datalen <= 0 || datalen > 32767 || !prep->data)
+ return -EINVAL;
+--- a/security/keys/gc.c
++++ b/security/keys/gc.c
+@@ -129,15 +129,15 @@ static noinline void key_gc_unused_keys(
+ while (!list_empty(keys)) {
+ struct key *key =
+ list_entry(keys->next, struct key, graveyard_link);
++ short state = key->state;
++
+ list_del(&key->graveyard_link);
+
+ kdebug("- %u", key->serial);
+ key_check(key);
+
+ /* Throw away the key data if the key is instantiated */
+- if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
+- !test_bit(KEY_FLAG_NEGATIVE, &key->flags) &&
+- key->type->destroy)
++ if (state == KEY_IS_POSITIVE && key->type->destroy)
+ key->type->destroy(key);
+
+ security_key_free(key);
+@@ -151,7 +151,7 @@ static noinline void key_gc_unused_keys(
+ }
+
+ atomic_dec(&key->user->nkeys);
+- if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
++ if (state != KEY_IS_UNINSTANTIATED)
+ atomic_dec(&key->user->nikeys);
+
+ key_user_put(key->user);
+--- a/security/keys/key.c
++++ b/security/keys/key.c
+@@ -401,6 +401,18 @@ int key_payload_reserve(struct key *key,
+ EXPORT_SYMBOL(key_payload_reserve);
+
+ /*
++ * Change the key state to being instantiated.
++ */
++static void mark_key_instantiated(struct key *key, int reject_error)
++{
++ /* Commit the payload before setting the state; barrier versus
++ * key_read_state().
++ */
++ smp_store_release(&key->state,
++ (reject_error < 0) ? reject_error : KEY_IS_POSITIVE);
++}
++
++/*
+ * Instantiate a key and link it into the target keyring atomically. Must be
+ * called with the target keyring's semaphore writelocked. The target key's
+ * semaphore need not be locked as instantiation is serialised by
+@@ -423,14 +435,14 @@ static int __key_instantiate_and_link(st
+ mutex_lock(&key_construction_mutex);
+
+ /* can't instantiate twice */
+- if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
++ if (key->state == KEY_IS_UNINSTANTIATED) {
+ /* instantiate the key */
+ ret = key->type->instantiate(key, prep);
+
+ if (ret == 0) {
+ /* mark the key as being instantiated */
+ atomic_inc(&key->user->nikeys);
+- set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
++ mark_key_instantiated(key, 0);
+
+ if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
+ awaken = 1;
+@@ -572,13 +584,10 @@ int key_reject_and_link(struct key *key,
+ mutex_lock(&key_construction_mutex);
+
+ /* can't instantiate twice */
+- if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
++ if (key->state == KEY_IS_UNINSTANTIATED) {
+ /* mark the key as being negatively instantiated */
+ atomic_inc(&key->user->nikeys);
+- key->reject_error = -error;
+- smp_wmb();
+- set_bit(KEY_FLAG_NEGATIVE, &key->flags);
+- set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
++ mark_key_instantiated(key, -error);
+ now = current_kernel_time();
+ key->expiry = now.tv_sec + timeout;
+ key_schedule_gc(key->expiry + key_gc_delay);
+@@ -750,8 +759,8 @@ static inline key_ref_t __key_update(key
+
+ ret = key->type->update(key, prep);
+ if (ret == 0)
+- /* updating a negative key instantiates it */
+- clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
++ /* Updating a negative key positively instantiates it */
++ mark_key_instantiated(key, 0);
+
+ up_write(&key->sem);
+
+@@ -995,8 +1004,8 @@ int key_update(key_ref_t key_ref, const
+
+ ret = key->type->update(key, &prep);
+ if (ret == 0)
+- /* updating a negative key instantiates it */
+- clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
++ /* Updating a negative key positively instantiates it */
++ mark_key_instantiated(key, 0);
+
+ up_write(&key->sem);
+
+--- a/security/keys/keyctl.c
++++ b/security/keys/keyctl.c
+@@ -766,10 +766,9 @@ long keyctl_read_key(key_serial_t keyid,
+
+ key = key_ref_to_ptr(key_ref);
+
+- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+- ret = -ENOKEY;
+- goto error2;
+- }
++ ret = key_read_state(key);
++ if (ret < 0)
++ goto error2; /* Negatively instantiated */
+
+ /* see if we can read it directly */
+ ret = key_permission(key_ref, KEY_NEED_READ);
+@@ -901,7 +900,7 @@ long keyctl_chown_key(key_serial_t id, u
+ atomic_dec(&key->user->nkeys);
+ atomic_inc(&newowner->nkeys);
+
+- if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
++ if (key->state != KEY_IS_UNINSTANTIATED) {
+ atomic_dec(&key->user->nikeys);
+ atomic_inc(&newowner->nikeys);
+ }
+--- a/security/keys/keyring.c
++++ b/security/keys/keyring.c
+@@ -407,7 +407,7 @@ static void keyring_describe(const struc
+ else
+ seq_puts(m, "[anon]");
+
+- if (key_is_instantiated(keyring)) {
++ if (key_is_positive(keyring)) {
+ if (keyring->keys.nr_leaves_on_tree != 0)
+ seq_printf(m, ": %lu", keyring->keys.nr_leaves_on_tree);
+ else
+@@ -546,7 +546,8 @@ static int keyring_search_iterator(const
+ {
+ struct keyring_search_context *ctx = iterator_data;
+ const struct key *key = keyring_ptr_to_key(object);
+- unsigned long kflags = key->flags;
++ unsigned long kflags = READ_ONCE(key->flags);
++ short state = READ_ONCE(key->state);
+
+ kenter("{%d}", key->serial);
+
+@@ -590,9 +591,8 @@ static int keyring_search_iterator(const
+
+ if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+ /* we set a different error code if we pass a negative key */
+- if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
+- smp_rmb();
+- ctx->result = ERR_PTR(key->reject_error);
++ if (state < 0) {
++ ctx->result = ERR_PTR(state);
+ kleave(" = %d [neg]", ctx->skipped_ret);
+ goto skipped;
+ }
+--- a/security/keys/proc.c
++++ b/security/keys/proc.c
+@@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_fil
+ unsigned long timo;
+ key_ref_t key_ref, skey_ref;
+ char xbuf[16];
++ short state;
+ int rc;
+
+ struct keyring_search_context ctx = {
+@@ -240,17 +241,19 @@ static int proc_keys_show(struct seq_fil
+ sprintf(xbuf, "%luw", timo / (60*60*24*7));
+ }
+
++ state = key_read_state(key);
++
+ #define showflag(KEY, LETTER, FLAG) \
+ (test_bit(FLAG, &(KEY)->flags) ? LETTER : '-')
+
+ seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ",
+ key->serial,
+- showflag(key, 'I', KEY_FLAG_INSTANTIATED),
++ state != KEY_IS_UNINSTANTIATED ? 'I' : '-',
+ showflag(key, 'R', KEY_FLAG_REVOKED),
+ showflag(key, 'D', KEY_FLAG_DEAD),
+ showflag(key, 'Q', KEY_FLAG_IN_QUOTA),
+ showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT),
+- showflag(key, 'N', KEY_FLAG_NEGATIVE),
++ state < 0 ? 'N' : '-',
+ showflag(key, 'i', KEY_FLAG_INVALIDATED),
+ atomic_read(&key->usage),
+ xbuf,
+--- a/security/keys/process_keys.c
++++ b/security/keys/process_keys.c
+@@ -729,7 +729,7 @@ try_again:
+
+ ret = -EIO;
+ if (!(lflags & KEY_LOOKUP_PARTIAL) &&
+- !test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
++ key_read_state(key) == KEY_IS_UNINSTANTIATED)
+ goto invalid_key;
+
+ /* check the permissions */
+--- a/security/keys/request_key.c
++++ b/security/keys/request_key.c
+@@ -594,10 +594,9 @@ int wait_for_key_construction(struct key
+ intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
+ if (ret)
+ return -ERESTARTSYS;
+- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+- smp_rmb();
+- return key->reject_error;
+- }
++ ret = key_read_state(key);
++ if (ret < 0)
++ return ret;
+ return key_validate(key);
+ }
+ EXPORT_SYMBOL(wait_for_key_construction);
+--- a/security/keys/request_key_auth.c
++++ b/security/keys/request_key_auth.c
+@@ -73,7 +73,7 @@ static void request_key_auth_describe(co
+
+ seq_puts(m, "key:");
+ seq_puts(m, key->description);
+- if (key_is_instantiated(key))
++ if (key_is_positive(key))
+ seq_printf(m, " pid:%d ci:%zu", rka->pid, rka->callout_len);
+ }
+
+--- a/security/keys/trusted.c
++++ b/security/keys/trusted.c
+@@ -1067,7 +1067,7 @@ static int trusted_update(struct key *ke
+ char *datablob;
+ int ret = 0;
+
+- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags))
++ if (key_is_negative(key))
+ return -ENOKEY;
+ p = key->payload.data[0];
+ if (!p->migratable)
+--- a/security/keys/user_defined.c
++++ b/security/keys/user_defined.c
+@@ -106,7 +106,7 @@ int user_update(struct key *key, struct
+
+ /* attach the new data, displacing the old */
+ key->expiry = prep->expiry;
+- if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
++ if (key_is_positive(key))
+ zap = rcu_dereference_key(key);
+ rcu_assign_keypointer(key, prep->payload.data[0]);
+ prep->payload.data[0] = NULL;
+@@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(user_destroy);
+ void user_describe(const struct key *key, struct seq_file *m)
+ {
+ seq_puts(m, key->description);
+- if (key_is_instantiated(key))
++ if (key_is_positive(key))
+ seq_printf(m, ": %u", key->datalen);
+ }
+
projects / thirdparty / kernel / stable-queue.git / commitdiff
