[RFC,01/17] fscrypt: factor accessing inode->i_crypt_info

Commit Message

Sweet Tea Dorminy Jan. 1, 2023, 5:06 a.m. UTC

Currently, inode->i_crypt_info is accessed directly in many places;
the initial setting occurs in one place, via cmpxchg_release, and
the initial access is abstracted into fscrypt_get_info() which uses
smp_load_acquire(), but there are many direct accesses. While many of
them follow calls to fscrypt_get_info() on the same thread, verifying
this is not always trivial.

For instance, fscrypt_crypt_block() does not obviously follow a call to
fscrypt_get_info() on the same cpu; if some other mechanism does not
ensure a memory barrier, it is conceivable that a filesystem could call
fscrypt_crypt_block() on a cpu which had an old (NULL) i_crypt_info
cached. Even if the cpu does READ_ONCE(i_crypt_info), I believe it's
theoretically possible for it to see the old NULL value, since this
could be happening on a cpu which did not do the smp_load_acquire().  (I
may be misunderstanding, but memory-barriers.txt says that only the cpus
involved in an acquire/release chain are guaranteed to see the correct
order of operations, which seems to imply that a cpu which does not do
an acquire may be able to see a memory value from before the release.)

For safety, then, and so each site doesn't need to be individually
evaluated, this change factors all accesses of i_crypt_info to go
through fscrypt_get_info(), ensuring every access uses acquire and is
thus paired against an appropriate release.

(The same treatment is not necessary for setting i_crypt_info; the
only unprotected setting is during inode cleanup, which is inevitably
followed by freeing the inode; there are no uses past the unprotected
setting possible.)

Signed-off-by: Sweet Tea Dorminy <[e-mail]>
---
 fs/crypto/crypto.c       |  2 +-
 fs/crypto/fname.c        |  9 +++++----
 fs/crypto/hooks.c        |  2 +-
 fs/crypto/inline_crypt.c | 10 +++++-----
 fs/crypto/keysetup.c     |  2 +-
 fs/crypto/policy.c       |  6 +++---
 6 files changed, 16 insertions(+), 15 deletions(-)

Comments

Eric Biggers Jan. 2, 2023, 9 p.m. UTC | #1

On Sun, Jan 01, 2023 at 12:06:05AM -0500, Sweet Tea Dorminy wrote:
> Currently, inode->i_crypt_info is accessed directly in many places;
> the initial setting occurs in one place, via cmpxchg_release, and
> the initial access is abstracted into fscrypt_get_info() which uses
> smp_load_acquire(), but there are many direct accesses. While many of
> them follow calls to fscrypt_get_info() on the same thread, verifying
> this is not always trivial.
> 
> For instance, fscrypt_crypt_block() does not obviously follow a call to
> fscrypt_get_info() on the same cpu; if some other mechanism does not
> ensure a memory barrier, it is conceivable that a filesystem could call
> fscrypt_crypt_block() on a cpu which had an old (NULL) i_crypt_info
> cached. Even if the cpu does READ_ONCE(i_crypt_info), I believe it's
> theoretically possible for it to see the old NULL value, since this
> could be happening on a cpu which did not do the smp_load_acquire().  (I
> may be misunderstanding, but memory-barriers.txt says that only the cpus
> involved in an acquire/release chain are guaranteed to see the correct
> order of operations, which seems to imply that a cpu which does not do
> an acquire may be able to see a memory value from before the release.)
> 
> For safety, then, and so each site doesn't need to be individually
> evaluated, this change factors all accesses of i_crypt_info to go
> through fscrypt_get_info(), ensuring every access uses acquire and is
> thus paired against an appropriate release.
> 
> (The same treatment is not necessary for setting i_crypt_info; the
> only unprotected setting is during inode cleanup, which is inevitably
> followed by freeing the inode; there are no uses past the unprotected
> setting possible.)
> 
> Signed-off-by: Sweet Tea Dorminy <[e-mail]>

This patch is not necessary.  The rules for accessing ->i_crypt_info are
actually pretty simple: when it's unknown whether ->i_crypt_info has been set,
then it's necessary to use fscrypt_get_info() and check whether the resulting
pointer is NULL or not (or use fscrypt_has_encryption_key() which does both).
That's because another thread could set it concurrently.

In contrast, when it *is* known that ->i_crypt_info has been set, then that can
only be because fscrypt_has_encryption_key() was already executed on the same
thread, or because an operation that ensured the key is set up already happened.
For example, when doing I/O to a file, it's guaranteed that the file has been
opened.  In either case, direct access is fine.

- Eric

Patch

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index e78be66bbf01..2efd1da9df8d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -107,7 +107,7 @@  int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist dst, src;
-	struct fscrypt_info *ci = inode->i_crypt_info;
+	struct fscrypt_info *ci = fscrypt_get_info(inode);
 	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
 	int res = 0;
 
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 12bd61d20f69..6efb53cba523 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -100,7 +100,7 @@  int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
 {
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
-	const struct fscrypt_info *ci = inode->i_crypt_info;
+	const struct fscrypt_info *ci = fscrypt_get_info(inode);
 	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
 	union fscrypt_iv iv;
 	struct scatterlist sg;
@@ -157,7 +157,7 @@  static int fname_decrypt(const struct inode *inode,
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist src_sg, dst_sg;
-	const struct fscrypt_info *ci = inode->i_crypt_info;
+	const struct fscrypt_info *ci = fscrypt_get_info(inode);
 	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
 	union fscrypt_iv iv;
 	int res;
@@ -299,7 +299,8 @@  bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
 bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
 				  u32 max_len, u32 *encrypted_len_ret)
 {
-	return __fscrypt_fname_encrypted_size(&inode->i_crypt_info->ci_policy,
+	struct fscrypt_info *ci = fscrypt_get_info(inode);
+	return __fscrypt_fname_encrypted_size(&ci->ci_policy,
 					      orig_len, max_len,
 					      encrypted_len_ret);
 }
@@ -568,7 +569,7 @@  EXPORT_SYMBOL_GPL(fscrypt_match_name);
  */
 u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name)
 {
-	const struct fscrypt_info *ci = dir->i_crypt_info;
+	const struct fscrypt_info *ci = fscrypt_get_info(dir);
 
 	WARN_ON(!ci->ci_dirhash_key_initialized);
 
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 7b8c5a1104b5..b605660fb3f1 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -152,7 +152,7 @@  int fscrypt_prepare_setflags(struct inode *inode,
 		err = fscrypt_require_key(inode);
 		if (err)
 			return err;
-		ci = inode->i_crypt_info;
+		ci = fscrypt_get_info(inode);
 		if (ci->ci_policy.version != FSCRYPT_POLICY_V2)
 			return -EINVAL;
 		mk = ci->ci_master_key;
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index cea8b14007e6..4b1373715018 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -232,7 +232,7 @@  void fscrypt_destroy_inline_crypt_key(struct super_block *sb,
 
 bool __fscrypt_inode_uses_inline_crypto(const struct inode *inode)
 {
-	return inode->i_crypt_info->ci_inlinecrypt;
+	return fscrypt_get_info(inode)->ci_inlinecrypt;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_inode_uses_inline_crypto);
 
@@ -274,7 +274,7 @@  void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 
 	if (!fscrypt_inode_uses_inline_crypto(inode))
 		return;
-	ci = inode->i_crypt_info;
+	ci = fscrypt_get_info(inode);
 
 	fscrypt_generate_dun(ci, first_lblk, dun);
 	bio_crypt_set_ctx(bio, ci->ci_enc_key.blk_key, dun, gfp_mask);
@@ -364,10 +364,10 @@  bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 	 * uses the same pointer.  I.e., there's currently no need to support
 	 * merging requests where the keys are the same but the pointers differ.
 	 */
-	if (bc->bc_key != inode->i_crypt_info->ci_enc_key.blk_key)
+	if (bc->bc_key != fscrypt_get_info(inode)->ci_enc_key.blk_key)
 		return false;
 
-	fscrypt_generate_dun(inode->i_crypt_info, next_lblk, next_dun);
+	fscrypt_generate_dun(fscrypt_get_info(inode), next_lblk, next_dun);
 	return bio_crypt_dun_is_contiguous(bc, bio->bi_iter.bi_size, next_dun);
 }
 EXPORT_SYMBOL_GPL(fscrypt_mergeable_bio);
@@ -469,7 +469,7 @@  u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
 	if (nr_blocks <= 1)
 		return nr_blocks;
 
-	ci = inode->i_crypt_info;
+	ci = fscrypt_get_info(inode);
 	if (!(fscrypt_policy_flags(&ci->ci_policy) &
 	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
 		return nr_blocks;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index f7407071a952..ad56192305b3 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -706,7 +706,7 @@  EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
  */
 void fscrypt_put_encryption_info(struct inode *inode)
 {
-	put_crypt_info(inode->i_crypt_info);
+	put_crypt_info(fscrypt_get_info(inode));
 	inode->i_crypt_info = NULL;
 }
 EXPORT_SYMBOL(fscrypt_put_encryption_info);
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 46757c3052ef..ccab27afd3cc 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -687,7 +687,7 @@  const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir)
 		err = fscrypt_require_key(dir);
 		if (err)
 			return ERR_PTR(err);
-		return &dir->i_crypt_info->ci_policy;
+		return &fscrypt_get_info(dir)->ci_policy;
 	}
 
 	return fscrypt_get_dummy_policy(dir->i_sb);
@@ -706,7 +706,7 @@  const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir)
  */
 int fscrypt_context_for_new_inode(void *ctx, struct inode *inode)
 {
-	struct fscrypt_info *ci = inode->i_crypt_info;
+	struct fscrypt_info *ci = fscrypt_get_info(inode);
 
 	BUILD_BUG_ON(sizeof(union fscrypt_context) !=
 			FSCRYPT_SET_CONTEXT_MAX_SIZE);
@@ -731,7 +731,7 @@  EXPORT_SYMBOL_GPL(fscrypt_context_for_new_inode);
  */
 int fscrypt_set_context(struct inode *inode, void *fs_data)
 {
-	struct fscrypt_info *ci = inode->i_crypt_info;
+	struct fscrypt_info *ci = fscrypt_get_info(inode);
 	union fscrypt_context ctx;
 	int ctxsize;