Commit ab673b98 authored by Eric Biggers's avatar Eric Biggers
Browse files

fscrypt: use smp_load_acquire() for ->i_crypt_info 

Normally smp_store_release() or cmpxchg_release() is paired with
smp_load_acquire().  Sometimes smp_load_acquire() can be replaced with
the more lightweight READ_ONCE().  However, for this to be safe, all the
published memory must only be accessed in a way that involves the
pointer itself.  This may not be the case if allocating the object also
involves initializing a static or global variable, for example.

fscrypt_info includes various sub-objects which are internal to and are
allocated by other kernel subsystems such as keyrings and crypto.  So by
using READ_ONCE() for ->i_crypt_info, we're relying on internal
implementation details of these other kernel subsystems.

Remove this fragile assumption by using smp_load_acquire() instead.

(Note: I haven't seen any real-world problems here.  This change is just
fixing the code to be guaranteed correct and less fragile.)

Fixes: e37a784d ("fscrypt: use READ_ONCE() to access ->i_crypt_info")
Link: https://lore.kernel.org/r/20200721225920.114347-5-ebiggers@kernel.org


Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
parent 777afe4e

fs/crypto/keysetup.c

+11 −1
Original line number Diff line number Diff line
@@ -518,7 +518,17 @@ int fscrypt_get_encryption_info(struct inode *inode) 
	if (res)

		goto out;



	/*

	 * Multiple tasks may race to set ->i_crypt_info, so use

	 * cmpxchg_release().  This pairs with the smp_load_acquire() in

	 * fscrypt_get_info().  I.e., here we publish ->i_crypt_info with a

	 * RELEASE barrier so that other tasks can ACQUIRE it.

	 */

	if (cmpxchg_release(&inode->i_crypt_info, NULL, crypt_info) == NULL) {

		/*

		 * We won the race and set ->i_crypt_info to our crypt_info.

		 * Now link it into the master key's inode list.

		 */

		if (master_key) {

			struct fscrypt_master_key *mk =

				master_key->payload.data[0];
@@ -589,7 +599,7 @@ EXPORT_SYMBOL(fscrypt_free_inode); 
 */

int fscrypt_drop_inode(struct inode *inode)

{

	const struct fscrypt_info *ci = READ_ONCE(inode->i_crypt_info);

	const struct fscrypt_info *ci = fscrypt_get_info(inode);

	const struct fscrypt_master_key *mk;



	/*

fs/crypto/policy.c

+2 −2
Original line number Diff line number Diff line
@@ -352,7 +352,7 @@ static int fscrypt_get_policy(struct inode *inode, union fscrypt_policy *policy) 
	union fscrypt_context ctx;

	int ret;



	ci = READ_ONCE(inode->i_crypt_info);

	ci = fscrypt_get_info(inode);

	if (ci) {

		/* key available, use the cached policy */

		*policy = ci->ci_policy;
@@ -641,7 +641,7 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child, 
	if (res < 0)

		return res;



	ci = READ_ONCE(parent->i_crypt_info);

	ci = fscrypt_get_info(parent);

	if (ci == NULL)

		return -ENOKEY;

include/linux/fscrypt.h

+24 −5
Original line number Diff line number Diff line
@@ -74,10 +74,15 @@ struct fscrypt_operations { 
			    struct request_queue **devs);

};



static inline bool fscrypt_has_encryption_key(const struct inode *inode)

static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode)

{

	/* pairs with cmpxchg_release() in fscrypt_get_encryption_info() */

	return READ_ONCE(inode->i_crypt_info) != NULL;

	/*

	 * Pairs with the cmpxchg_release() in fscrypt_get_encryption_info().

	 * I.e., another task may publish ->i_crypt_info concurrently, executing

	 * a RELEASE barrier.  We need to use smp_load_acquire() here to safely

	 * ACQUIRE the memory the other task published.

	 */

	return smp_load_acquire(&inode->i_crypt_info);

}



/**
@@ -234,9 +239,9 @@ static inline void fscrypt_set_ops(struct super_block *sb, 
}

#else  /* !CONFIG_FS_ENCRYPTION */



static inline bool fscrypt_has_encryption_key(const struct inode *inode)

static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode)

{

	return false;

	return NULL;

}



static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
@@ -619,6 +624,20 @@ static inline bool fscrypt_inode_uses_fs_layer_crypto(const struct inode *inode) 
	       !__fscrypt_inode_uses_inline_crypto(inode);

}



/**

 * fscrypt_has_encryption_key() - check whether an inode has had its key set up

 * @inode: the inode to check

 *

 * Return: %true if the inode has had its encryption key set up, else %false.

 *

 * Usually this should be preceded by fscrypt_get_encryption_info() to try to

 * set up the key first.

 */

static inline bool fscrypt_has_encryption_key(const struct inode *inode)

{

	return fscrypt_get_info(inode) != NULL;

}



/**

 * fscrypt_require_key() - require an inode's encryption key

 * @inode: the inode we need the key for

CRA Git | Maintained and supported by SUSTech CRA and CCSE