KEYS: Fix race between updating and finding a negative key

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>

diff --git a/include/linux/key.h b/include/linux/key.h
index dcc115e8dd03d725cdd12fc6e2e3cde3ae51d8ca..af071ca73079dd328ca8e8ef9036c7f6c43c1132 100644 (file)
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -126,6 +126,11 @@ static inline bool is_key_possessed(const key_ref_t key_ref)
        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,19 +171,17 @@ 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_TRUSTED       8       /* set if key is trusted */
-#define KEY_FLAG_TRUSTED_ONLY  9       /* set if keyring only accepts links to trusted keys */
-#define KEY_FLAG_BUILTIN       10      /* set if key is builtin */
-#define KEY_FLAG_ROOT_CAN_INVAL        11      /* set if key can be invalidated by root without permission */
-#define KEY_FLAG_UID_KEYRING   12      /* 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_TRUSTED       6       /* set if key is trusted */
+#define KEY_FLAG_TRUSTED_ONLY  7       /* set if keyring only accepts links to trusted keys */
+#define KEY_FLAG_BUILTIN       8       /* set if key is builtin */
+#define KEY_FLAG_ROOT_CAN_INVAL        9       /* set if key can be invalidated by root without permission */
+#define KEY_FLAG_UID_KEYRING   10      /* 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
@@ -203,7 +207,6 @@ struct key {
                        struct list_head name_link;
                        struct assoc_array keys;
                };
-               int reject_error;
        };
 };
 
@@ -319,17 +322,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 rcu_dereference_key(KEY)                                       \

diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index c79b85eb4d4ca4712465b6bbeda7ed74d2624344..6abc5012200b6ee7f5f15a96345b2b13e5a1ba03 100644 (file)
--- 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 907c1522ee469b05d42bcf9a839c0a80bf7eff6c..08c4cc5c2973bc18074ef9bbb42ae2e128fffc52 100644 (file)
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -138,7 +138,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);
 }
@@ -170,7 +170,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 dbd75de136d725f255ee83144029f34dd24d4baa..ce295c0c1da0bb24de99d9f553b2cc3a5cc37b2a 100644 (file)
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -852,7 +852,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 9cb4fe4478a137e7bb3352ae958830657f6b5b7c..1659094d684db34c722c43fd89d506ff1a1d8cb1 100644 (file)
--- 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 2751ab4a7946ca3ec286e34174577cd2c4c6f52e..4d971bf88ac337574a25dc9a11b8eb04b153fac4 100644 (file)
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -395,6 +395,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
@@ -418,14 +430,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;
@@ -553,13 +565,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);
@@ -731,8 +740,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);
 
@@ -967,8 +976,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 a009dc66eb8f65a1074ca8bdbeea85aacadf8a29..2e741e1a8712cf195007b4d0e9ff52392b866e93 100644 (file)
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -738,10 +738,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);
@@ -873,7 +872,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 0c8dd4fbe130c8797b149e9c4f90647089a17c10..ef828238cdc03d8888c9adbaa5362142c36627d6 100644 (file)
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -407,7 +407,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
@@ -522,7 +522,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);
 
@@ -566,9 +567,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 b9f531c9e4fa753d326752b63dc2cf599579ffeb..0361286824638d97f048058abaaed555e7a4f55a 100644 (file)
--- 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),
                   atomic_read(&key->usage),
                   xbuf,

diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 7dd050f2426122f703f223669e7ac660f4d32871..ac1d5b2b1626c0e86453f9bbfdb4e814a8301135 100644 (file)
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -727,7 +727,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 */

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index c7a117c9a8f3030d66afc6cbf5767ce5069c813b..2ce733342b5a18ac0d8495d843db23d6434de441 100644 (file)
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -594,10 +594,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 4f0f112fe276fd7da1714e65113753394b5453e0..217775fcd0f3eed48715e4def15b4364b6faf506 100644 (file)
--- 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 16dec53184b663f745c010d11e78128ca995bf58..509aedcf83104d63d4e9c04e532811c31f7ad6c2 100644 (file)
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1014,7 +1014,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 8705d79b2c6f289736fde21fd38e6013a4e4ae3c..eba8a516ee9ea1bd770ef5ffb267a6bbb42a28c1 100644 (file)
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -120,7 +120,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 
        if (ret == 0) {
                /* attach the new data, displacing the old */
-               if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags))
+               if (key_is_positive(key))
                        zap = key->payload.data[0];
                else
                        zap = NULL;
@@ -174,7 +174,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);
 }