[PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, add_key() will, when passed a key that already exists, call
the key's ->update() method.  But this is heavily broken in the case
where the key is uninstantiated because it doesn't call
__key_instantiate_and_link().  Consequently, it doesn't do most of the
things that are supposed to happen when the key is instantiated, such as
setting KEY_FLAG_INSTANTIATED, clearing KEY_FLAG_USER_CONSTRUCT and
awakening tasks waiting on it, and incrementing key->user->nikeys.

It also never takes key_construction_mutex, which means that
->instantiate() can run concurrently with ->update() on the same key.
In the case of the "user" and "logon" key types this causes a memory
leak, at best.  Maybe even worse, the ->update() methods of the
"encrypted" and "trusted" key types actually just dereference a NULL
pointer when passed an uninstantiated key.

Therefore, change find_key_to_update() to return NULL if the found key
is uninstantiated, so that add_key() replaces the key rather than
instantiating it.  This seems to be better than fixing __key_update() to
call __key_instantiate_and_link(), since given all the bugs noted above
as well as that the existing behavior was undocumented and
keyctl_instantiate() is supposed to be used instead, I doubt anyone was
relying on the existing behavior.

This patch only affects *uninstantiated* keys.  For now we still allow a
negatively instantiated key to be updated (thereby positively
instantiating it), although that's broken too (the next patch fixes it)
and I'm not sure that anyone actually uses that functionality either.

Here is a simple reproducer for the bug using the "encrypted" key type
(requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug
pertained to more than just the "encrypted" key type:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                const char payload[] = "update user:foo 32";

                usleep(rand() % 10000);
                add_key("encrypted", "desc", payload, sizeof(payload), ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("encrypted", "desc", "callout_info", ringid);
        }
    }

It causes:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
    IP: encrypted_update+0xb0/0x170
    PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0
    PREEMPT SMP
    CPU: 0 PID: 340 Comm: reproduce Tainted: G      D         4.14.0-rc1-00025-g428490e38b2e #796
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    task: ffff8a467a39a340 task.stack: ffffb15c40770000
    RIP: 0010:encrypted_update+0xb0/0x170
    RSP: 0018:ffffb15c40773de8 EFLAGS: 00010246
    RAX: 0000000000000000 RBX: ffff8a467a275b00 RCX: 0000000000000000
    RDX: 0000000000000005 RSI: ffff8a467a275b14 RDI: ffffffffb742f303
    RBP: ffffb15c40773e20 R08: 0000000000000000 R09: ffff8a467a275b17
    R10: 0000000000000020 R11: 0000000000000000 R12: 0000000000000000
    R13: 0000000000000000 R14: ffff8a4677057180 R15: ffff8a467a275b0f
    FS:  00007f5d7fb08700(0000) GS:ffff8a467f200000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000000018 CR3: 0000000077262005 CR4: 00000000001606f0
    Call Trace:
     key_create_or_update+0x2bc/0x460
     SyS_add_key+0x10c/0x1d0
     entry_SYSCALL_64_fastpath+0x1f/0xbe
    RIP: 0033:0x7f5d7f211259
    RSP: 002b:00007ffed03904c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
    RAX: ffffffffffffffda RBX: 000000003b2a7955 RCX: 00007f5d7f211259
    RDX: 00000000004009e4 RSI: 00000000004009ff RDI: 0000000000400a04
    RBP: 0000000068db8bad R08: 000000003b2a7955 R09: 0000000000000004
    R10: 000000000000001a R11: 0000000000000246 R12: 0000000000400868
    R13: 00007ffed03905d0 R14: 0000000000000000 R15: 0000000000000000
    Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b
    RIP: encrypted_update+0xb0/0x170 RSP: ffffb15c40773de8
    CR2: 0000000000000018

Cc: <stable@vger.kernel.org>    [v2.6.12+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyring.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..129a4175760b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1056,8 +1056,8 @@ EXPORT_SYMBOL(keyring_restrict);
  * caller must also hold a lock on the keyring semaphore.
  *
  * Returns a pointer to the found key with usage count incremented if
- * successful and returns NULL if not found.  Revoked and invalidated keys are
- * skipped over.
+ * successful and returns NULL if not found.  Revoked, invalidated, and
+ * uninstantiated keys are skipped over.  (But negative keys are not!)
  *
  * If successful, the possession indicator is propagated from the keyring ref
  * to the returned key reference.
@@ -1084,8 +1084,10 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref,
 
 found:
 	key = keyring_ptr_to_key(object);
-	if (key->flags & ((1 << KEY_FLAG_INVALIDATED) |
-			  (1 << KEY_FLAG_REVOKED))) {
+	if ((key->flags & ((1 << KEY_FLAG_INVALIDATED) |
+			   (1 << KEY_FLAG_REVOKED) |
+			   (1 << KEY_FLAG_INSTANTIATED))) !=
+	    (1 << KEY_FLAG_INSTANTIATED)) {
 		kleave(" = NULL [x]");
 		return NULL;
 	}
-- 
2.14.2.822.g60be5d43e6-goog

[PATCH v3 2/7] KEYS: fix race between updating and finding negative key

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

In keyring_search_iterator() and in wait_for_key_construction(), we
check whether the key has been negatively instantiated, and if so return
the key's ->reject_error.

However, no lock is held during this, and ->reject_error is in union
with ->payload.  And it's impossible for KEY_FLAG_NEGATIVE to be updated
atomically with respect to ->reject_error and ->payload.

Most problematically, when a negative key is positively instantiated via
__key_update() (via sys_add_key()), ->payload is initialized first, then
KEY_FLAG_NEGATIVE is cleared.  But that means that ->reject_error can be
observed to have a bogus value, having been overwritten with ->payload,
while the key still appears to be "negative".  Clearing
KEY_FLAG_NEGATIVE first wouldn't work either, since then anyone who
accesses the payload under rcu_read_lock() rather than the key semaphore
might observe an uninitialized ->payload.  Nor can we just always take
the key's semaphore when checking whether the key is negative, since
keyring searches happen under rcu_read_lock().

Therefore, fix the bug by moving ->reject_error into the high bits of
->flags so that we can read and write it atomically with respect to
KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.

This will also allow KEY_FLAG_NEGATIVE to be removed, since tests for
KEY_FLAG_NEGATIVE can be replaced with tests for nonzero reject_error.
But for ease of backporting this fix, that is left for a later patch.

This fixes a kernel crash caused by the following program:

    #include <stdlib.h>
    #include <unistd.h>
    #include <keyutils.h>

    int main(void)
    {
        int ringid = keyctl_join_session_keyring(NULL);

        if (fork()) {
            for (;;) {
                usleep(rand() % 4096);
                add_key("user", "desc", "x", 1, ringid);
                keyctl_clear(ringid);
            }
        } else {
            for (;;)
                request_key("user", "desc", "", ringid);
        }
    }

Here is the crash:

    BUG: unable to handle kernel paging request at fffffffffd39a6b0
    IP: __key_link_begin+0x0/0x100
    PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 1 PID: 165 Comm: keyctl_negate_r Not tainted 4.14.0-rc1 #377
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
    task: ffff9791fd809140 task.stack: ffffacba402bc000
    RIP: 0010:__key_link_begin+0x0/0x100
    RSP: 0018:ffffacba402bfdc8 EFLAGS: 00010282
    RAX: ffff9791fd809140 RBX: fffffffffd39a620 RCX: 0000000000000008
    RDX: ffffacba402bfdd0 RSI: fffffffffd39a6a0 RDI: ffff9791fd810600
    RBP: ffffacba402bfdf8 R08: 0000000000000063 R09: ffffffff94845620
    R10: 8080808080808080 R11: 0000000000000004 R12: ffff9791fd810600
    R13: ffff9791fd39a940 R14: fffffffffd39a6a0 R15: 0000000000000000
    FS:  00007fbf14a90740(0000) GS:ffff9791ffd00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: fffffffffd39a6b0 CR3: 000000003b910003 CR4: 00000000003606e0
    Call Trace:
     ? key_link+0x28/0xb0
     ? search_process_keyrings+0x13/0x100
     request_key_and_link+0xcb/0x550
     ? keyring_instantiate+0x110/0x110
     ? key_default_cmp+0x20/0x20
     SyS_request_key+0xc0/0x160
     ? exit_to_usermode_loop+0x5e/0x80
     entry_SYSCALL_64_fastpath+0x1a/0xa5
    RIP: 0033:0x7fbf14190bb9
    RSP: 002b:00007ffd8e4fe6c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f9
    RAX: ffffffffffffffda RBX: 0000000036cc28fb RCX: 00007fbf14190bb9
    RDX: 000055748b56ca4a RSI: 000055748b56ca46 RDI: 000055748b56ca4b
    RBP: 000055748b56ca4a R08: 0000000000000001 R09: 0000000000000001
    R10: 0000000036cc28fb R11: 0000000000000246 R12: 000055748b56c8b0
    R13: 00007ffd8e4fe7d0 R14: 0000000000000000 R15: 0000000000000000
    Code: c5 0f 85 69 ff ff ff 48 c7 c3 82 ff ff ff eb ab 45 31 ed e9 18 ff ff ff 85 c0 75 8d eb d2 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7e 10 00 0f 84 c5 00 00 00 55 48 89 e5 41 57 41 56 41 55
    RIP: __key_link_begin+0x0/0x100 RSP: ffffacba402bfdc8
    CR2: fffffffffd39a6b0

Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: <stable@vger.kernel.org>	[v4.4+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h         | 12 +++++++++++-
 security/keys/key.c         | 26 +++++++++++++++++++-------
 security/keys/keyctl.c      |  3 +++
 security/keys/keyring.c     |  4 ++--
 security/keys/request_key.c | 11 +++++++----
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..b7b590d7c480 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -189,6 +189,17 @@ struct key {
 #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 */
 
+	/*
+	 * If the key is negatively instantiated, then bits 20-31 hold the error
+	 * code which should be returned when someone tries to use the key
+	 * (unless they allow negative keys).  The error code is stored as a
+	 * positive number, so it must be negated before being returned.
+	 *
+	 * Note that a key can go from negative to positive but not vice versa.
+	 */
+#define KEY_FLAGS_REJECT_ERROR_SHIFT	20
+#define KEY_FLAGS_REJECT_ERROR_MASK	0xFFF00000
+
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
 	 * - it should be a printable string
@@ -213,7 +224,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
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..786158d3442e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -401,6 +401,20 @@ int key_payload_reserve(struct key *key, size_t datalen)
 }
 EXPORT_SYMBOL(key_payload_reserve);
 
+static void mark_key_instantiated(struct key *key, unsigned int reject_error)
+{
+	unsigned long old, new;
+
+	do {
+		old = READ_ONCE(key->flags);
+		new = (old & ~((1 << KEY_FLAG_NEGATIVE) |
+			       KEY_FLAGS_REJECT_ERROR_MASK)) |
+		      (1 << KEY_FLAG_INSTANTIATED) |
+		      (reject_error ? (1 << KEY_FLAG_NEGATIVE) : 0) |
+		      (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
+	} while (cmpxchg_release(&key->flags, old, new) != old);
+}
+
 /*
  * 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
@@ -431,7 +445,7 @@ static int __key_instantiate_and_link(struct key *key,
 		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;
@@ -580,10 +594,8 @@ int key_reject_and_link(struct key *key,
 	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
 		/* 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);
@@ -753,7 +765,7 @@ 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);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
@@ -987,7 +999,7 @@ 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);
+		mark_key_instantiated(key, 0);
 
 	up_write(&key->sem);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..19a09e121089 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1223,6 +1223,9 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
 	    error == ERESTART_RESTARTBLOCK)
 		return -EINVAL;
 
+	BUILD_BUG_ON(MAX_ERRNO > (KEY_FLAGS_REJECT_ERROR_MASK >>
+				  KEY_FLAGS_REJECT_ERROR_SHIFT));
+
 	/* the appropriate instantiation authorisation key must have been
 	 * assumed before calling this */
 	ret = -EPERM;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 129a4175760b..e54ad0ed7aa4 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -598,8 +598,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);
+			ctx->result = ERR_PTR(-(int)(kflags >>
+						KEY_FLAGS_REJECT_ERROR_SHIFT));
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..0aab68344837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -590,15 +590,18 @@ struct key *request_key_and_link(struct key_type *type,
 int wait_for_key_construction(struct key *key, bool intr)
 {
 	int ret;
+	unsigned long flags;
 
 	ret = wait_on_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT,
 			  intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 	if (ret)
 		return -ERESTARTSYS;
-	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
-		smp_rmb();
-		return key->reject_error;
-	}
+
+	/* Pairs with RELEASE in mark_key_instantiated() */
+	flags = smp_load_acquire(&key->flags);
+	if (flags & (1 << KEY_FLAG_NEGATIVE))
+		return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
+
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
-- 
2.14.2.822.g60be5d43e6-goog

Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> +	if ((key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> +			   (1 << KEY_FLAG_REVOKED) |
> +			   (1 << KEY_FLAG_INSTANTIATED))) !=
> +	    (1 << KEY_FLAG_INSTANTIATED)) {

Does this need READ_ONCE(), I wonder?

David

Re: [PATCH v3 2/7] KEYS: fix race between updating and finding negative key

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> Therefore, fix the bug by moving ->reject_error into the high bits of
> ->flags so that we can read and write it atomically with respect to
> KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.

I would rather not eat up that much of the flags word.  I would rather
something like the attached patch, which allows both the NEGATIVE and
INSTANTIATED flags to be eliminated.

Changing the state member is always done under a write lock on the key
semaphore, and it might be possible to roll in the REVOKE and DEAD flags as
separate states also.

David
---
commit b23bda640e9a9bfe66bb3384d1b9db85ad701e04
Author: David Howells <dhowells@redhat.com>
Date:   Wed Oct 4 16:43:25 2017 +0100

    KEYS: Fix race between updating and finding a negative key
    
    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.
    
    Possibly the revocation flags can also be combined with this also.
    
    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>

diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..f50e88c52bd0 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_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,18 +182,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
@@ -213,7 +217,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
@@ -362,8 +365,12 @@ extern void key_set_timeout(struct key *, unsigned);
  */
 static inline bool key_is_instantiated(const struct key *key)
 {
-	return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) &&
-		!test_bit(KEY_FLAG_NEGATIVE, &key->flags);
+	return key->state == KEY_IS_INSTANTIATED;
+}
+
+static inline bool key_is_negative(const struct key *key)
+{
+	return key->state < 0;
 }
 
 #define dereference_key_rcu(KEY)					\
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 535db141f4da..d92cbf9687c3 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..1578f671a213 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -129,14 +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 = READ_ONCE(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) &&
+		if (state == KEY_IS_INSTANTIATED &&
 		    key->type->destroy)
 			key->type->destroy(key);
 
@@ -151,7 +152,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_INSTANTIATED)
 			atomic_dec(&key->user->nikeys);
 
 		key_user_put(key->user);
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..de1b789ad29f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -402,6 +402,15 @@ 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)
+{
+	smp_wmb(); /* Commit the payload before setting the state */
+	key->state = (reject_error < 0) ? reject_error : KEY_IS_INSTANTIATED;
+}
+
+/*
  * 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
@@ -424,14 +433,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;
@@ -577,13 +586,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);
@@ -752,8 +758,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);
 
@@ -986,8 +992,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 365ff85d7e27..5c30c54e01ed 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->state;
+	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_INSTANTIATED) {
 			atomic_dec(&key->user->nikeys);
 			atomic_inc(&newowner->nikeys);
 		}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..816948abff89 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -553,7 +553,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);
 
@@ -597,9 +598,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 de834309d100..46fa0f1bfcc3 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 = {
@@ -236,17 +237,19 @@ static int proc_keys_show(struct seq_file *m, void *v)
 			sprintf(xbuf, "%luw", timo / (60*60*24*7));
 	}
 
+	state = READ_ONCE(key->state);
+
 #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_INSTANTIATED ? '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,
@@ -257,8 +260,10 @@ static int proc_keys_show(struct seq_file *m, void *v)
 
 #undef showflag
 
-	if (key->type->describe)
+	if (key->type->describe) {
+		smp_rmb(); /* Order access to payload after state set. */
 		key->type->describe(key, m);
+	}
 	seq_putc(m, '\n');
 
 	rcu_read_unlock();
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 293d3598153b..492217ff4924 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -729,9 +729,11 @@ 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))
-		goto invalid_key;
+	if (!(lflags & KEY_LOOKUP_PARTIAL)) {
+		if (key->state != KEY_IS_INSTANTIATED)
+			goto invalid_key;
+		smp_rmb(); /* Order access to payload after state set. */
+	}
 
 	/* check the permissions */
 	ret = key_task_permission(key_ref, ctx.cred, perm);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..a25f8378f704 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -595,10 +595,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 = READ_ONCE(key->state);
+	if (ret < 0)
+		return ret;
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ddfaebf60fc8..bd85315cbfeb 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..9afa64817d4f 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->state < 0)
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;

Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> Therefore, change find_key_to_update() to return NULL if the found key
> is uninstantiated, so that add_key() replaces the key rather than
> instantiating it.  This seems to be better than fixing __key_update() to
> call __key_instantiate_and_link(), since given all the bugs noted above
> as well as that the existing behavior was undocumented and
> keyctl_instantiate() is supposed to be used instead, I doubt anyone was
> relying on the existing behavior.

keyctl_instantiate() can only be called from an upcall.  It can't be called in
the same context as keyctl_update().

I would be okay with making key_update() wait for completion of construction
in this case.

David