Commit 363b02da authored by David Howells's avatar David Howells
Browse files

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.

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: 146aa8b1 ("KEYS: Merge the type-specific data with the payload data")
Cc: stable@vger.kernel.org # v4.4+
Reported-by: default avatarEric Biggers <ebiggers@google.com>
Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
Reviewed-by: default avatarEric Biggers <ebiggers@google.com>
parent b3811d36

include/linux/key.h

+30 −17
Original line number Diff line number Diff line
@@ -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,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
@@ -353,17 +356,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)					\

net/dns_resolver/dns_key.c

+1 −1
Original line number Diff line number Diff line
@@ -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)

security/keys/big_key.c

+2 −2
Original line number Diff line number Diff line
@@ -247,7 +247,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);

}
@@ -279,7 +279,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");

security/keys/encrypted-keys/encrypted.c

+1 −1
Original line number Diff line number Diff line
@@ -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;

security/keys/gc.c

+4 −4
Original line number Diff line number Diff line
@@ -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);

CRA Git | Maintained and supported by SUSTech CRA and CCSE