Home Home > GIT Browse > SLE12-SP4
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichal Suchanek <msuchanek@suse.de>2018-01-29 14:02:39 +0100
committerMichal Suchanek <msuchanek@suse.de>2018-01-29 14:02:40 +0100
commitd6cb5a23b2d31dccc81fac8f44f795e103ee7eef (patch)
tree76810b8481786a6ddbb916ef8f244ff107dc4874
parent280e241317012e5ebf71d573e6dc41a78863884e (diff)
KEYS: Fix race between updating and finding a negative key
(CVE-2017-15951, bsc#1065615, bsc#1071927).
-rw-r--r--patches.fixes/KEYS-Fix-race-between-updating-and-finding-a-negativ.patch496
-rw-r--r--series.conf1
2 files changed, 497 insertions, 0 deletions
diff --git a/patches.fixes/KEYS-Fix-race-between-updating-and-finding-a-negativ.patch b/patches.fixes/KEYS-Fix-race-between-updating-and-finding-a-negativ.patch
new file mode 100644
index 0000000000..6a5b7683c1
--- /dev/null
+++ b/patches.fixes/KEYS-Fix-race-between-updating-and-finding-a-negativ.patch
@@ -0,0 +1,496 @@
+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: [PATCH] KEYS: Fix race between updating and finding a negative key
+
+References: CVE-2017-15951, bsc#1065615, bsc#1071927
+Patch-mainline: v4.14-rc6
+Git-commit: 363b02dab09b3226f3bd1420dad9c72b79a42a76
+
+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")
+Cc: stable@vger.kernel.org # v4.4+
+Reported-by: Eric Biggers <ebiggers@google.com>
+Signed-off-by: David Howells <dhowells@redhat.com>
+Reviewed-by: Eric Biggers <ebiggers@google.com>
+Acked-by: Michal Suchanek <msuchanek@suse.de>
+---
+ include/linux/key.h | 45 ++++++++++++++++++++------------
+ 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, 79 insertions(+), 56 deletions(-)
+
+diff --git a/include/linux/key.h b/include/linux/key.h
+index 78e25aabedaf..7d9c3a8e0e4d 100644
+--- a/include/linux/key.h
++++ b/include/linux/key.h
+@@ -138,6 +138,11 @@ struct key_restriction {
+ struct key_type *keytype;
+ };
+
++enum key_state {
++ KEY_IS_UNINSTANTIATED,
++ KEY_IS_POSITIVE, /* Positively instantiated */
++};
++
+ /*****************************************************************************/
+ /*
+ * authentication token / access credential / keyring
+@@ -169,6 +174,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;
+@@ -176,17 +182,15 @@ 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_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 */
+
+ /* the key type and key description string
+ * - the desc is used to match a key against search criteria
+@@ -212,7 +216,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
+@@ -351,17 +354,27 @@ extern void key_set_timeout(struct key *, unsigned);
+ #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 dereference_key_rcu(KEY) \
+diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
+index 8737412c7b27..e1d4d898a007 100644
+--- a/net/dns_resolver/dns_key.c
++++ b/net/dns_resolver/dns_key.c
+@@ -224,7 +224,7 @@ static int dns_resolver_match_preparse(struct key_match_data *match_data)
+ 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)
+diff --git a/security/keys/big_key.c b/security/keys/big_key.c
+index f3e71b44901c..b83703a624d8 100644
+--- a/security/keys/big_key.c
++++ b/security/keys/big_key.c
+@@ -240,7 +240,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);
+ }
+@@ -272,7 +272,7 @@ void big_key_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, ": %zu [%s]",
+ datalen,
+ datalen > BIG_KEY_FILE_THRESHOLD ? "file" : "buff");
+diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
+index bd64a3991e26..fea053611618 100644
+--- a/security/keys/encrypted-keys/encrypted.c
++++ b/security/keys/encrypted-keys/encrypted.c
+@@ -854,7 +854,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
+ 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;
+diff --git a/security/keys/gc.c b/security/keys/gc.c
+index 87cb260e4890..f01d48cb3de1 100644
+--- a/security/keys/gc.c
++++ b/security/keys/gc.c
+@@ -129,15 +129,15 @@ static noinline void key_gc_unused_keys(struct list_head *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(struct list_head *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);
+diff --git a/security/keys/key.c b/security/keys/key.c
+index bec11eefe04d..146584383b1d 100644
+--- a/security/keys/key.c
++++ b/security/keys/key.c
+@@ -400,6 +400,18 @@ int key_payload_reserve(struct key *key, size_t datalen)
+ 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
+@@ -422,14 +434,14 @@ static int __key_instantiate_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) {
+ /* 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;
+@@ -575,13 +587,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_ref_t key_ref,
+
+ 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);
+
+@@ -994,8 +1003,8 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
+
+ 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);
+
+diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
+index 6a82090c7fc1..2eb624c0aefc 100644
+--- a/security/keys/keyctl.c
++++ b/security/keys/keyctl.c
+@@ -766,10 +766,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
+
+ 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, uid_t user, gid_t group)
+ 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);
+ }
+diff --git a/security/keys/keyring.c b/security/keys/keyring.c
+index b9893fb68e4a..78e2c7b04013 100644
+--- a/security/keys/keyring.c
++++ b/security/keys/keyring.c
+@@ -414,7 +414,7 @@ static void keyring_describe(const struct key *keyring, struct seq_file *m)
+ 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
+@@ -552,7 +552,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
+ {
+ 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);
+
+@@ -596,9 +597,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
+
+ 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;
+ }
+diff --git a/security/keys/proc.c b/security/keys/proc.c
+index bf08d02b6646..e6aa1b257578 100644
+--- a/security/keys/proc.c
++++ b/security/keys/proc.c
+@@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
+ 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_file *m, void *v)
+ 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),
+ refcount_read(&key->usage),
+ xbuf,
+diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
+index 86bced9fdbdf..241f57c59371 100644
+--- a/security/keys/process_keys.c
++++ b/security/keys/process_keys.c
+@@ -728,7 +728,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
+
+ 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 */
+diff --git a/security/keys/request_key.c b/security/keys/request_key.c
+index 3d2272c06cae..5054e3196073 100644
+--- a/security/keys/request_key.c
++++ b/security/keys/request_key.c
+@@ -623,10 +623,9 @@ int wait_for_key_construction(struct key *key, bool intr)
+ 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);
+diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
+index 0f062156dfb2..4e48ca3690a5 100644
+--- a/security/keys/request_key_auth.c
++++ b/security/keys/request_key_auth.c
+@@ -73,7 +73,7 @@ static void request_key_auth_describe(const struct key *key,
+
+ 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);
+ }
+
+diff --git a/security/keys/trusted.c b/security/keys/trusted.c
+index 5a775f2fbe15..e0fcb17068f5 100644
+--- a/security/keys/trusted.c
++++ b/security/keys/trusted.c
+@@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+ 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)
+diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
+index 3d8c68eba516..9f558bedba23 100644
+--- a/security/keys/user_defined.c
++++ b/security/keys/user_defined.c
+@@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
+
+ /* 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 = dereference_key_locked(key);
+ rcu_assign_keypointer(key, prep->payload.data[0]);
+ prep->payload.data[0] = NULL;
+@@ -162,7 +162,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);
+ }
+
+--
+2.13.6
+
diff --git a/series.conf b/series.conf
index 84d2de542d..cf5a78431a 100644
--- a/series.conf
+++ b/series.conf
@@ -7027,6 +7027,7 @@
##########################################################
patches.fixes/KEYS-don-t-let-add_key-update-an-uninstantiated-key.patch
+ patches.fixes/KEYS-Fix-race-between-updating-and-finding-a-negativ.patch
##########################################################
# crypto