linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] fscrypt, fs-verity: one-time init fixes
@ 2020-07-21 22:59 Eric Biggers
  2020-07-21 22:59 ` [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library Eric Biggers
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, Daniel Rosenberg,
	Satya Tangirala

This series fixes up some cases in fs/crypto/ and fs/verity/ where
"one-time init" is implemented using READ_ONCE() instead of
smp_load_acquire() but it's not obviously correct.

One case is fixed by using a better approach that removes the need to
initialize anything.  The others are fixed by upgrading READ_ONCE() to
smp_load_acquire().  I've also improved the comments.

This is motivated by the discussions at 
https://lkml.kernel.org/linux-fsdevel/20200713033330.205104-1-ebiggers@kernel.org/T/#u
and
https://lkml.kernel.org/linux-fsdevel/20200717044427.68747-1-ebiggers@kernel.org/T/#u

These fixes are improvements over the status quo, so I'd prefer to apply
them now, without waiting for any potential new generic one-time-init
macros (which based on the latest discussion, won't be flexible enough
to handle most of these cases anyway).

Eric Biggers (5):
  fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library
  fscrypt: use smp_load_acquire() for fscrypt_prepared_key
  fscrypt: use smp_load_acquire() for ->s_master_keys
  fscrypt: use smp_load_acquire() for ->i_crypt_info
  fs-verity: use smp_load_acquire() for ->i_verity_info

 fs/crypto/Kconfig           |  2 +-
 fs/crypto/fname.c           | 41 +++++++++----------------------------
 fs/crypto/fscrypt_private.h | 15 ++++++++------
 fs/crypto/inline_crypt.c    |  6 ++++--
 fs/crypto/keyring.c         | 15 +++++++++++---
 fs/crypto/keysetup.c        | 18 +++++++++++++---
 fs/crypto/policy.c          |  4 ++--
 fs/verity/open.c            | 15 +++++++++++---
 include/linux/fscrypt.h     | 29 +++++++++++++++++++++-----
 include/linux/fsverity.h    |  9 ++++++--
 10 files changed, 96 insertions(+), 58 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
@ 2020-07-21 22:59 ` Eric Biggers
  2020-07-21 22:59 ` [PATCH 2/5] fscrypt: use smp_load_acquire() for fscrypt_prepared_key Eric Biggers
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

fscrypt_do_sha256() is only used for hashing encrypted filenames to
create no-key tokens, which isn't performance-critical.  Therefore a C
implementation of SHA-256 is sufficient.

Also, the logic to create no-key tokens is always potentially needed.
This differs from fscrypt's other dependencies on crypto API algorithms,
which are conditionally needed depending on what encryption policies
userspace is using.  Therefore, for fscrypt there isn't much benefit to
allowing SHA-256 to be a loadable module.

So, make fscrypt_do_sha256() use the SHA-256 library instead of the
crypto_shash API.  This is much simpler, since it avoids having to
implement one-time-init (which is hard to do correctly, and in fact was
implemented incorrectly) and handle failures to allocate the
crypto_shash object.

Fixes: edc440e3d27f ("fscrypt: improve format of no-key names")
Cc: Daniel Rosenberg <drosen@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/Kconfig |  2 +-
 fs/crypto/fname.c | 41 ++++++++++-------------------------------
 2 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index f1f11a6228eb..a5f5c30368a2 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -4,6 +4,7 @@ config FS_ENCRYPTION
 	select CRYPTO
 	select CRYPTO_HASH
 	select CRYPTO_SKCIPHER
+	select CRYPTO_LIB_SHA256
 	select KEYS
 	help
 	  Enable encryption of files and directories.  This
@@ -21,7 +22,6 @@ config FS_ENCRYPTION_ALGS
 	select CRYPTO_CTS
 	select CRYPTO_ECB
 	select CRYPTO_HMAC
-	select CRYPTO_SHA256
 	select CRYPTO_SHA512
 	select CRYPTO_XTS
 
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index d828e3df898b..011830f84d8d 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -61,30 +61,13 @@ struct fscrypt_nokey_name {
  */
 #define FSCRYPT_NOKEY_NAME_MAX	offsetofend(struct fscrypt_nokey_name, sha256)
 
-static struct crypto_shash *sha256_hash_tfm;
-
-static int fscrypt_do_sha256(const u8 *data, unsigned int data_len, u8 *result)
+static void fscrypt_do_sha256(const u8 *data, unsigned int data_len, u8 *result)
 {
-	struct crypto_shash *tfm = READ_ONCE(sha256_hash_tfm);
-
-	if (unlikely(!tfm)) {
-		struct crypto_shash *prev_tfm;
-
-		tfm = crypto_alloc_shash("sha256", 0, 0);
-		if (IS_ERR(tfm)) {
-			fscrypt_err(NULL,
-				    "Error allocating SHA-256 transform: %ld",
-				    PTR_ERR(tfm));
-			return PTR_ERR(tfm);
-		}
-		prev_tfm = cmpxchg(&sha256_hash_tfm, NULL, tfm);
-		if (prev_tfm) {
-			crypto_free_shash(tfm);
-			tfm = prev_tfm;
-		}
-	}
+	struct sha256_state sctx;
 
-	return crypto_shash_tfm_digest(tfm, data, data_len, result);
+	sha256_init(&sctx);
+	sha256_update(&sctx, data, data_len);
+	sha256_final(&sctx, result);
 }
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
@@ -349,7 +332,6 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 	const struct qstr qname = FSTR_TO_QSTR(iname);
 	struct fscrypt_nokey_name nokey_name;
 	u32 size; /* size of the unencoded no-key name */
-	int err;
 
 	if (fscrypt_is_dot_dotdot(&qname)) {
 		oname->name[0] = '.';
@@ -387,11 +369,9 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 	} else {
 		memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes));
 		/* Compute strong hash of remaining part of name. */
-		err = fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
-					iname->len - sizeof(nokey_name.bytes),
-					nokey_name.sha256);
-		if (err)
-			return err;
+		fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
+				  iname->len - sizeof(nokey_name.bytes),
+				  nokey_name.sha256);
 		size = FSCRYPT_NOKEY_NAME_MAX;
 	}
 	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
@@ -530,9 +510,8 @@ bool fscrypt_match_name(const struct fscrypt_name *fname,
 		return false;
 	if (memcmp(de_name, nokey_name->bytes, sizeof(nokey_name->bytes)))
 		return false;
-	if (fscrypt_do_sha256(&de_name[sizeof(nokey_name->bytes)],
-			      de_name_len - sizeof(nokey_name->bytes), sha256))
-		return false;
+	fscrypt_do_sha256(&de_name[sizeof(nokey_name->bytes)],
+			  de_name_len - sizeof(nokey_name->bytes), sha256);
 	return !memcmp(sha256, nokey_name->sha256, sizeof(sha256));
 }
 EXPORT_SYMBOL_GPL(fscrypt_match_name);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/5] fscrypt: use smp_load_acquire() for fscrypt_prepared_key
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
  2020-07-21 22:59 ` [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library Eric Biggers
@ 2020-07-21 22:59 ` Eric Biggers
  2020-07-21 22:59 ` [PATCH 3/5] fscrypt: use smp_load_acquire() for ->s_master_keys Eric Biggers
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

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_prepared_key includes a pointer to a crypto_skcipher object,
which is internal to and is allocated by the crypto subsystem.  By using
READ_ONCE() for it, we're relying on internal implementation details of
the crypto subsystem.

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: 5fee36095cda ("fscrypt: add inline encryption support")
Cc: Satya Tangirala <satyat@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h | 15 +++++++++------
 fs/crypto/inline_crypt.c    |  6 ++++--
 fs/crypto/keysetup.c        |  6 ++++--
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index bc1a3fcd45ed..8117a61b6f55 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -351,13 +351,16 @@ fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_info *ci)
 {
 	/*
-	 * The READ_ONCE() here pairs with the smp_store_release() in
-	 * fscrypt_prepare_key().  (This only matters for the per-mode keys,
-	 * which are shared by multiple inodes.)
+	 * The two smp_load_acquire()'s here pair with the smp_store_release()'s
+	 * in fscrypt_prepare_inline_crypt_key() and fscrypt_prepare_key().
+	 * I.e., in some cases (namely, if this prep_key is a per-mode
+	 * encryption key) another task can publish blk_key or tfm concurrently,
+	 * executing a RELEASE barrier.  We need to use smp_load_acquire() here
+	 * to safely ACQUIRE the memory the other task published.
 	 */
 	if (fscrypt_using_inline_encryption(ci))
-		return READ_ONCE(prep_key->blk_key) != NULL;
-	return READ_ONCE(prep_key->tfm) != NULL;
+		return smp_load_acquire(&prep_key->blk_key) != NULL;
+	return smp_load_acquire(&prep_key->tfm) != NULL;
 }
 
 #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
@@ -391,7 +394,7 @@ static inline bool
 fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_info *ci)
 {
-	return READ_ONCE(prep_key->tfm) != NULL;
+	return smp_load_acquire(&prep_key->tfm) != NULL;
 }
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index d7aecadf33c1..dfb06375099a 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -176,8 +176,10 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 		}
 	}
 	/*
-	 * Pairs with READ_ONCE() in fscrypt_is_key_prepared().  (Only matters
-	 * for the per-mode keys, which are shared by multiple inodes.)
+	 * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared().
+	 * I.e., here we publish ->blk_key with a RELEASE barrier so that
+	 * concurrent tasks can ACQUIRE it.  Note that this concurrency is only
+	 * possible for per-mode keys, not for per-file keys.
 	 */
 	smp_store_release(&prep_key->blk_key, blk_key);
 	return 0;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 22a94b18fe70..7f85fc645602 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -129,8 +129,10 @@ int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key,
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 	/*
-	 * Pairs with READ_ONCE() in fscrypt_is_key_prepared().  (Only matters
-	 * for the per-mode keys, which are shared by multiple inodes.)
+	 * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared().
+	 * I.e., here we publish ->tfm with a RELEASE barrier so that
+	 * concurrent tasks can ACQUIRE it.  Note that this concurrency is only
+	 * possible for per-mode keys, not for per-file keys.
 	 */
 	smp_store_release(&prep_key->tfm, tfm);
 	return 0;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/5] fscrypt: use smp_load_acquire() for ->s_master_keys
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
  2020-07-21 22:59 ` [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library Eric Biggers
  2020-07-21 22:59 ` [PATCH 2/5] fscrypt: use smp_load_acquire() for fscrypt_prepared_key Eric Biggers
@ 2020-07-21 22:59 ` Eric Biggers
  2020-07-21 22:59 ` [PATCH 4/5] fscrypt: use smp_load_acquire() for ->i_crypt_info Eric Biggers
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel

From: Eric Biggers <ebiggers@google.com>

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.

super_block::s_master_keys is a keyring, which is internal to and is
allocated by the keyrings subsystem.  By using READ_ONCE() for it, we're
relying on internal implementation details of the keyrings subsystem.

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: 22d94f493bfb ("fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/keyring.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 7f8ac61a20d6..71d56f8e2870 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -213,7 +213,11 @@ static int allocate_filesystem_keyring(struct super_block *sb)
 	if (IS_ERR(keyring))
 		return PTR_ERR(keyring);
 
-	/* Pairs with READ_ONCE() in fscrypt_find_master_key() */
+	/*
+	 * Pairs with the smp_load_acquire() in fscrypt_find_master_key().
+	 * I.e., here we publish ->s_master_keys with a RELEASE barrier so that
+	 * concurrent tasks can ACQUIRE it.
+	 */
 	smp_store_release(&sb->s_master_keys, keyring);
 	return 0;
 }
@@ -234,8 +238,13 @@ struct key *fscrypt_find_master_key(struct super_block *sb,
 	struct key *keyring;
 	char description[FSCRYPT_MK_DESCRIPTION_SIZE];
 
-	/* pairs with smp_store_release() in allocate_filesystem_keyring() */
-	keyring = READ_ONCE(sb->s_master_keys);
+	/*
+	 * Pairs with the smp_store_release() in allocate_filesystem_keyring().
+	 * I.e., another task can publish ->s_master_keys concurrently,
+	 * executing a RELEASE barrier.  We need to use smp_load_acquire() here
+	 * to safely ACQUIRE the memory the other task published.
+	 */
+	keyring = smp_load_acquire(&sb->s_master_keys);
 	if (keyring == NULL)
 		return ERR_PTR(-ENOKEY); /* No keyring yet, so no keys yet. */
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/5] fscrypt: use smp_load_acquire() for ->i_crypt_info
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
                   ` (2 preceding siblings ...)
  2020-07-21 22:59 ` [PATCH 3/5] fscrypt: use smp_load_acquire() for ->s_master_keys Eric Biggers
@ 2020-07-21 22:59 ` Eric Biggers
  2020-07-21 22:59 ` [PATCH 5/5] fs-verity: use smp_load_acquire() for ->i_verity_info Eric Biggers
  2020-07-27 16:38 ` [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel

From: Eric Biggers <ebiggers@google.com>

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: e37a784d8b6a ("fscrypt: use READ_ONCE() to access ->i_crypt_info")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/keysetup.c    | 12 +++++++++++-
 fs/crypto/policy.c      |  4 ++--
 include/linux/fscrypt.h | 29 ++++++++++++++++++++++++-----
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 7f85fc645602..fea6226afc2b 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -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;
 
 	/*
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 8a8ad0e44bb8..2a2d0c06147b 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -338,7 +338,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;
@@ -627,7 +627,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;
 
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index bb257411365f..991ff8575d0e 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -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
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 5/5] fs-verity: use smp_load_acquire() for ->i_verity_info
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
                   ` (3 preceding siblings ...)
  2020-07-21 22:59 ` [PATCH 4/5] fscrypt: use smp_load_acquire() for ->i_crypt_info Eric Biggers
@ 2020-07-21 22:59 ` Eric Biggers
  2020-07-27 16:38 ` [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-21 22:59 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel

From: Eric Biggers <ebiggers@google.com>

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.

fsverity_info::tree_params.hash_alg->tfm is a crypto_ahash object that's
internal to and is allocated by the crypto subsystem.  So by using
READ_ONCE() for ->i_verity_info, we're relying on internal
implementation details of the crypto subsystem.

Remove this fragile assumption by using smp_load_acquire() instead.

Also fix the cmpxchg logic to correctly execute an ACQUIRE barrier when
losing the cmpxchg race, since cmpxchg doesn't guarantee a memory
barrier on failure.

(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: fd2d1acfcadf ("fs-verity: add the hook for file ->open()")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/open.c         | 15 ++++++++++++---
 include/linux/fsverity.h |  9 +++++++--
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/verity/open.c b/fs/verity/open.c
index d007db0c9304..bfe0280c14e4 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -221,11 +221,20 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
 void fsverity_set_info(struct inode *inode, struct fsverity_info *vi)
 {
 	/*
-	 * Multiple processes may race to set ->i_verity_info, so use cmpxchg.
-	 * This pairs with the READ_ONCE() in fsverity_get_info().
+	 * Multiple tasks may race to set ->i_verity_info, so use
+	 * cmpxchg_release().  This pairs with the smp_load_acquire() in
+	 * fsverity_get_info().  I.e., here we publish ->i_verity_info with a
+	 * RELEASE barrier so that other tasks can ACQUIRE it.
 	 */
-	if (cmpxchg(&inode->i_verity_info, NULL, vi) != NULL)
+	if (cmpxchg_release(&inode->i_verity_info, NULL, vi) != NULL) {
+		/* Lost the race, so free the fsverity_info we allocated. */
 		fsverity_free_info(vi);
+		/*
+		 * Afterwards, the caller may access ->i_verity_info directly,
+		 * so make sure to ACQUIRE the winning fsverity_info.
+		 */
+		(void)fsverity_get_info(inode);
+	}
 }
 
 void fsverity_free_info(struct fsverity_info *vi)
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 78201a6d35f6..c1144a450392 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -115,8 +115,13 @@ struct fsverity_operations {
 
 static inline struct fsverity_info *fsverity_get_info(const struct inode *inode)
 {
-	/* pairs with the cmpxchg() in fsverity_set_info() */
-	return READ_ONCE(inode->i_verity_info);
+	/*
+	 * Pairs with the cmpxchg_release() in fsverity_set_info().
+	 * I.e., another task may publish ->i_verity_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_verity_info);
 }
 
 /* enable.c */
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/5] fscrypt, fs-verity: one-time init fixes
  2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
                   ` (4 preceding siblings ...)
  2020-07-21 22:59 ` [PATCH 5/5] fs-verity: use smp_load_acquire() for ->i_verity_info Eric Biggers
@ 2020-07-27 16:38 ` Eric Biggers
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-07-27 16:38 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, Daniel Rosenberg,
	Satya Tangirala

On Tue, Jul 21, 2020 at 03:59:15PM -0700, Eric Biggers wrote:
> This series fixes up some cases in fs/crypto/ and fs/verity/ where
> "one-time init" is implemented using READ_ONCE() instead of
> smp_load_acquire() but it's not obviously correct.
> 
> One case is fixed by using a better approach that removes the need to
> initialize anything.  The others are fixed by upgrading READ_ONCE() to
> smp_load_acquire().  I've also improved the comments.
> 
> This is motivated by the discussions at 
> https://lkml.kernel.org/linux-fsdevel/20200713033330.205104-1-ebiggers@kernel.org/T/#u
> and
> https://lkml.kernel.org/linux-fsdevel/20200717044427.68747-1-ebiggers@kernel.org/T/#u
> 
> These fixes are improvements over the status quo, so I'd prefer to apply
> them now, without waiting for any potential new generic one-time-init
> macros (which based on the latest discussion, won't be flexible enough
> to handle most of these cases anyway).
> 
> Eric Biggers (5):
>   fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library
>   fscrypt: use smp_load_acquire() for fscrypt_prepared_key
>   fscrypt: use smp_load_acquire() for ->s_master_keys
>   fscrypt: use smp_load_acquire() for ->i_crypt_info
>   fs-verity: use smp_load_acquire() for ->i_verity_info
> 
>  fs/crypto/Kconfig           |  2 +-
>  fs/crypto/fname.c           | 41 +++++++++----------------------------
>  fs/crypto/fscrypt_private.h | 15 ++++++++------
>  fs/crypto/inline_crypt.c    |  6 ++++--
>  fs/crypto/keyring.c         | 15 +++++++++++---
>  fs/crypto/keysetup.c        | 18 +++++++++++++---
>  fs/crypto/policy.c          |  4 ++--
>  fs/verity/open.c            | 15 +++++++++++---
>  include/linux/fscrypt.h     | 29 +++++++++++++++++++++-----
>  include/linux/fsverity.h    |  9 ++++++--
>  10 files changed, 96 insertions(+), 58 deletions(-)

Patches 1-4 applied to fscrypt.git#master for 5.9.
Patch 5 applied to fscrypt.git#fsverity for 5.9.

- Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-07-27 16:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 22:59 [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers
2020-07-21 22:59 ` [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library Eric Biggers
2020-07-21 22:59 ` [PATCH 2/5] fscrypt: use smp_load_acquire() for fscrypt_prepared_key Eric Biggers
2020-07-21 22:59 ` [PATCH 3/5] fscrypt: use smp_load_acquire() for ->s_master_keys Eric Biggers
2020-07-21 22:59 ` [PATCH 4/5] fscrypt: use smp_load_acquire() for ->i_crypt_info Eric Biggers
2020-07-21 22:59 ` [PATCH 5/5] fs-verity: use smp_load_acquire() for ->i_verity_info Eric Biggers
2020-07-27 16:38 ` [PATCH 0/5] fscrypt, fs-verity: one-time init fixes Eric Biggers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).