[PATCH] KEYS: fix race between updating and finding negative key

@@ -181,7 +181,6 @@ struct key {
 #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 */

@@ -213,9 +212,22 @@ struct key {
 			struct list_head name_link;
 			struct assoc_array keys;
 		};
-		int reject_error;
 	};
 
+	/*
+	 * This is set to a negative error value if the key is negatively
+	 * instantiated.
+	 *
+	 * This could in theory be in union with ->payload, with a separate flag
+	 * indicating that the key is negative.  But that doesn't work because
+	 * sometimes we need to check this without holding a lock.
+	 *
+	 * Note that keys can go from negative to positive but not vice versa.
+	 * That is, if KEY_FLAG_INSTANTIATED is set and this field is 0, then
+	 * the key is positive, and it cannot turn negative out from under you.
+	 */
+	int reject_error;
+
 	/* This is set on a keyring to restrict the addition of a link to a key
 	 * to it.  If this structure isn't provided then it is assumed that the
 	 * keyring is open to any addition.  It is ignored for non-keyring

@@ -353,6 +365,18 @@ 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 */
 
+/**
+ * key_is_negative - Determine if a key has been negatively instantiated
+ * @key: The key to check.
+ *
+ * Return true if the specified key has been negatively instantiated, false
+ * otherwise.
+ */
+static inline bool key_is_negative(const struct key *key)
+{
+	return READ_ONCE(key->reject_error) != 0;
+}
+
 /**
  * key_is_instantiated - Determine if a key has been positively instantiated
  * @key: The key to check.

@@ -362,8 +386,10 @@ 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);
+	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
+		return false;
+	smp_rmb(); /* pairs with smp_wmb() in key_reject_and_link() */
+	return !key_is_negative(key);
 }
 
 #define dereference_key_rcu(KEY)					\

@@ -847,7 +847,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;

@@ -135,9 +135,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 		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 (key_is_instantiated(key) && key->type->destroy)
 			key->type->destroy(key);
 
 		security_key_free(key);

@@ -578,11 +578,17 @@ int key_reject_and_link(struct key *key,
 
 	/* can't instantiate twice */
 	if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
-		/* mark the key as being negatively instantiated */
 		atomic_inc(&key->user->nikeys);
-		key->reject_error = -error;
+
+		/* mark the key as being negatively instantiated */
+		WRITE_ONCE(key->reject_error, -error);
+
+		/*
+		 * pairs with smp_rmb() in key_is_instantiated() and
+		 * wait_for_key_construction()
+		 */
 		smp_wmb();
-		set_bit(KEY_FLAG_NEGATIVE, &key->flags);
+
 		set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
 		now = current_kernel_time();
 		key->expiry = now.tv_sec + timeout;

@@ -753,7 +759,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);
+		WRITE_ONCE(key->reject_error, 0);
 
 	up_write(&key->sem);
 

@@ -987,7 +993,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);
+		WRITE_ONCE(key->reject_error, 0);
 
 	up_write(&key->sem);
 

@@ -766,7 +766,7 @@ 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)) {
+	if (key_is_negative(key)) {
 		ret = -ENOKEY;
 		goto error2;
 	}

@@ -597,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);
+		ctx->result = ERR_PTR(READ_ONCE(key->reject_error));
+		if (ctx->result) {
 			kleave(" = %d [neg]", ctx->skipped_ret);
 			goto skipped;
 		}

@@ -246,7 +246,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 		   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),
+		   key_is_negative(key) ? 'N' : '-',
 		   showflag(key, 'i', KEY_FLAG_INVALIDATED),
 		   refcount_read(&key->usage),
 		   xbuf,

@@ -595,10 +595,10 @@ 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;
-	}
+	smp_rmb(); /* pairs with smp_wmb() in key_reject_and_link() */
+	ret = READ_ONCE(key->reject_error);
+	if (ret)
+		return ret;
 	return key_validate(key);
 }
 EXPORT_SYMBOL(wait_for_key_construction);

@@ -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)

@@ -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_negative(key))
 		zap = dereference_key_locked(key);
 	rcu_assign_keypointer(key, prep->payload.data[0]);
 	prep->payload.data[0] = NULL;