fscrypt: d_revalidate fixes and cleanups

PATCH In review Eric Biggers 3 participants 15 messages Mar 20, 2019 – Apr 17, 2019

Subsystems (6)

VFS Memory technology devices (MTD) ext4 file system F2FS file system +2 more subsystems

lore

Share & tools

lore

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #0 lore raw

v2 cover fscrypt: d_revalidate fixes and cleanups

From: Eric Biggers <[email protected]>

This patch series improves dentry revalidation in fscrypt.

To recap, fscrypt (aka ext4/f2fs/ubifs encryption) encrypts both file contents and file names in individual directory trees. A single filesystem can contain many encrypted directory trees using many different encryption keys. Major users of fscrypt require the ability to delete encrypted files when their encryption key is unavailable, e.g. when the system needs to delete a removed user's home directory or free up space from a logged-out user's cache directory.

Therefore fscrypt allows listing, looking up, and deleting files in encrypted directories via encoded ciphertext names, but only before the key is added. After the key is added, the ciphertext names are invalidated via ->d_revalidate() and plaintext names are shown instead.

fscrypt isn't a stacked filesystem, and it's explicitly for storage encryption, not OS-level access control. Thus, whether each directory inode has its key or not is a global state, not per-process. Also, the inode keeps its key until it's evicted from the inode cache. So, plaintext names shouldn't ever get invalidated by ->d_revalidate().

This patch series makes the following improvements:

- Only assign ->d_revalidate() to ciphertext filenames, thus allowing overlayfs to use an fscrypt-encrypted upperdir in some cases. (Previous discussion: https://lkml.org/lkml/2019/3/13/255)

- Fix cases where plaintext filenames would wrongly be invalidated, including a real-world bug recently reported on Chromium OS.

- Fix cases where ciphertext filenames would wrongly not be invalidated.

- Allow rcu-walk lookups in encrypted directories with the key, which improves performance.
(Previous attempt: https://patchwork.kernel.org/patch/10594133/)

- Fix cases where rename() and link() could succeed on ciphertext names.

Changed since v1:

- Fixed comment in fscrypt_d_revalidate() to explain that dget_parent() is actually still required.

- Moved clearing DCACHE_ENCRYPTED_NAME into fscrypt.h, to avoid an extra #ifdef and cluttering up dcache.c.

Eric Biggers (5):
fscrypt: clean up and improve dentry revalidation
fscrypt: fix race allowing rename() and link() of ciphertext dentries fs, fscrypt: clear DCACHE_ENCRYPTED_NAME when unaliasing directory
fscrypt: only set dentry_operations on ciphertext dentries
fscrypt: fix race where ->lookup() marks plaintext dentry as
ciphertext

 fs/crypto/crypto.c      | 58 ++++++++++++++++---------------
 fs/crypto/fname.c       |  1 +
 fs/crypto/hooks.c       | 28 ++++++++++-----
 fs/dcache.c             |  2 ++
 fs/ext4/ext4.h          | 62 +++++++++++++++++++++++++--------
 fs/ext4/namei.c         | 76 ++++++++++++++++++++++++++++-------------
 fs/f2fs/namei.c         | 17 +++++----
 fs/ubifs/dir.c          |  8 ++---
 include/linux/dcache.h  |  2 +-
 include/linux/fscrypt.h | 61 +++++++++++++++++++++++----------
 10 files changed, 208 insertions(+), 107 deletions(-)

Signature · 1 line

2.21.0.225.g810b269d1ac-goog

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #1 lore raw

PATCH v2 1/5 fscrypt: clean up and improve dentry revalidation

From: Eric Biggers <[email protected]>

Make various improvements to fscrypt dentry revalidation:

- Don't try to handle the case where the per-directory key is removed, as this can't happen without the inode (and dentries) being evicted.

- Flag ciphertext dentries rather than plaintext dentries, since it's ciphertext dentries that need the special handling.

- Avoid doing unnecessary work for non-ciphertext dentries.

- When revalidating ciphertext dentries, try to set up the directory's i_crypt_info to make sure the key is really still absent, rather than invalidating all negative dentries as the previous code did. An old comment suggested we can't do this for locking reasons, but AFAICT this comment was outdated and it actually works fine.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/crypto.c      | 58 +++++++++++++++++++++--------------------
 fs/crypto/hooks.c       |  4 +--
 include/linux/dcache.h  |  2 +-
 include/linux/fscrypt.h |  6 ++---
 4 files changed, 35 insertions(+), 35 deletions(-)

fs/crypto/crypto.c +30 ~~-28~~

index 4dc788e3bc96b..3fc84bf2b1e52 100644
@@ -313,45 +313,47 @@ int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
 EXPORT_SYMBOL(fscrypt_decrypt_page);
 
 /*
- * Validate dentries for encrypted directories to make sure we aren't
- * potentially caching stale data after a key has been added or
- * removed.
+ * Validate dentries in encrypted directories to make sure we aren't potentially
+ * caching stale dentries after a key has been added.
  */
 static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *dir;
-	int dir_has_key, cached_with_key;
+	int err;
+	int valid;
+
+	/*
+	 * Plaintext names are always valid, since fscrypt doesn't support
+	 * reverting to ciphertext names without evicting the directory's inode
+	 * -- which implies eviction of the dentries in the directory.
+	 */
+	if (!(dentry->d_flags & DCACHE_ENCRYPTED_NAME))
+		return 1;
+
+	/*
+	 * Ciphertext name; valid if the directory's key is still unavailable.
+	 *
+	 * Although fscrypt forbids rename() on ciphertext names, we still must
+	 * use dget_parent() here rather than use ->d_parent directly.  That's
+	 * because a corrupted fs image may contain directory hard links, which
+	 * the VFS handles by moving the directory's dentry tree in the dcache
+	 * each time ->lookup() finds the directory and it already has a dentry
+	 * elsewhere.  Thus ->d_parent can be changing, and we must safely grab
+	 * a reference to some ->d_parent to prevent it from being freed.
+	 */
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
 	dir = dget_parent(dentry);
-	if (!IS_ENCRYPTED(d_inode(dir))) {
-		dput(dir);
-		return 0;
-	}
-
-	spin_lock(&dentry->d_lock);
-	cached_with_key = dentry->d_flags & DCACHE_ENCRYPTED_WITH_KEY;
-	spin_unlock(&dentry->d_lock);
-	dir_has_key = (d_inode(dir)->i_crypt_info != NULL);
+	err = fscrypt_get_encryption_info(d_inode(dir));
+	valid = (d_inode(dir)->i_crypt_info == NULL);
 	dput(dir);
 
-	/*
-	 * If the dentry was cached without the key, and it is a
-	 * negative dentry, it might be a valid name.  We can't check
-	 * if the key has since been made available due to locking
-	 * reasons, so we fail the validation so ext4_lookup() can do
-	 * this check.
-	 *
-	 * We also fail the validation if the dentry was created with
-	 * the key present, but we no longer have the key, or vice versa.
-	 */
-	if ((!cached_with_key && d_is_negative(dentry)) ||
-			(!cached_with_key && dir_has_key) ||
-			(cached_with_key && !dir_has_key))
-		return 0;
-	return 1;
+	if (err < 0)
+		return err;
+
+	return valid;
 }
 
 const struct dentry_operations fscrypt_d_ops = {

fs/crypto/hooks.c +2 -2

index 56debb1fcf5eb..a9492f75bbe13 100644
@@ -101,9 +101,9 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry)
 	if (err)
 		return err;
 
-	if (fscrypt_has_encryption_key(dir)) {
+	if (!fscrypt_has_encryption_key(dir)) {
 		spin_lock(&dentry->d_lock);
-		dentry->d_flags |= DCACHE_ENCRYPTED_WITH_KEY;
+		dentry->d_flags |= DCACHE_ENCRYPTED_NAME;
 		spin_unlock(&dentry->d_lock);
 	}

include/linux/dcache.h +1 -1

index 60996e64c5798..9b3b75d3bd211 100644
@@ -212,7 +212,7 @@ struct dentry_operations {
 
 #define DCACHE_MAY_FREE			0x00800000
 #define DCACHE_FALLTHRU			0x01000000 /* Fall through to lower layer */
-#define DCACHE_ENCRYPTED_WITH_KEY	0x02000000 /* dir is encrypted with a valid key */
+#define DCACHE_ENCRYPTED_NAME		0x02000000 /* Encrypted name (dir key was unavailable) */
 #define DCACHE_OP_REAL			0x04000000
 
 #define DCACHE_PAR_LOOKUP		0x10000000 /* being looked up (with parent locked shared) */

include/linux/fscrypt.h +2 -4

index e5194fc3983e9..5a0c3fee1ea2b 100644
@@ -545,10 +545,8 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
  * filenames are presented in encrypted form.  Therefore, we'll try to set up
  * the directory's encryption key, but even without it the lookup can continue.
  *
- * To allow invalidating stale dentries if the directory's encryption key is
- * added later, we also install a custom ->d_revalidate() method and use the
- * DCACHE_ENCRYPTED_WITH_KEY flag to indicate whether a given dentry is a
- * plaintext name (flag set) or a ciphertext name (flag cleared).
+ * This also installs a custom ->d_revalidate() method which will invalidate the
+ * dentry if it was created without the key and the key is later added.
  *
  * Return: 0 on success, -errno if a problem occurred while setting up the
  * encryption key

Signature · 8 lines

2.21.0.225.g810b269d1ac-goog



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2019-04-16 23:08 UTC #2 lore raw

Re: [PATCH v2 1/5] fscrypt: clean up and improve dentry revalidation

Quoted 12 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Make various improvements to fscrypt dentry revalidation:
- Don't try to handle the case where the per-directory key is removed, as this can't happen without the inode (and dentries) being evicted.
- Flag ciphertext dentries rather than plaintext dentries, since it's ciphertext dentries that need the special handling.
- Avoid doing unnecessary work for non-ciphertext dentries.

Quoted 7 lines

- When revalidating ciphertext dentries, try to set up the directory's i_crypt_info to make sure the key is really still absent, rather than invalidating all negative dentries as the previous code did. An old comment suggested we can't do this for locking reasons, but AFAICT this comment was outdated and it actually works fine.
Signed-off-by Eric Biggers <[email protected]>

Looks good, applied.

- Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Eric Biggers

author maintainer

2019-04-17 00:10 UTC #3 lore raw

Re: [PATCH v2 1/5] fscrypt: clean up and improve dentry revalidation

Quoted 19 lines · Theodore Ts'o

Quoted 18 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Make various improvements to fscrypt dentry revalidation:
- Don't try to handle the case where the per-directory key is removed, as this can't happen without the inode (and dentries) being evicted.
- Flag ciphertext dentries rather than plaintext dentries, since it's ciphertext dentries that need the special handling.
- Avoid doing unnecessary work for non-ciphertext dentries.
- When revalidating ciphertext dentries, try to set up the directory's i_crypt_info to make sure the key is really still absent, rather than invalidating all negative dentries as the previous code did. An old comment suggested we can't do this for locking reasons, but AFAICT this comment was outdated and it actually works fine.

Quoted 5 lines

Quoted 1 line
Signed-off-by Eric Biggers <[email protected]>
Looks good, applied.
- Ted

Hi Ted, I assumed you resolved the conflict with "fscrypt: use READ_ONCE() to access ->i_crypt_info"? The code in fscrypt_d_revalidate() should be:

        dir = dget_parent(dentry);
        err = fscrypt_get_encryption_info(d_inode(dir));
        valid = !fscrypt_has_encryption_key(d_inode(dir));
        dput(dir);

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2019-04-17 13:50 UTC #4 lore raw

Re: [PATCH v2 1/5] fscrypt: clean up and improve dentry revalidation

Quoted 8 lines · Eric Biggers

Hi Ted, I assumed you resolved the conflict with "fscrypt: use READ_ONCE() to access ->i_crypt_info"? The code in fscrypt_d_revalidate() should be:
        dir = dget_parent(dentry);
        err = fscrypt_get_encryption_info(d_inode(dir));
        valid = !fscrypt_has_encryption_key(d_inode(dir));
        dput(dir);

Yes, I did notice the patch conflict. Thanks for confirming the correct resolution!

- Ted

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #5 lore raw

PATCH v2 2/5 fscrypt: fix race allowing rename() and link() of ciphertext dentries

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Close some race conditions where fscrypt allowed rename() and link() on ciphertext dentries that had been looked up just prior to the key being concurrently added. It's better to return -ENOKEY in this case.

This avoids doing the nonsensical thing of encrypting the names a second time when searching for the actual on-disk dir entries. It also guarantees that DCACHE_ENCRYPTED_NAME dentries are never rename()d, so the dcache won't have support all possible combinations of moving DCACHE_ENCRYPTED_NAME around during __d_move().

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/hooks.c       | 12 +++++++++++-
 include/linux/fscrypt.h |  9 +++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

fs/crypto/hooks.c +11 -1

index a9492f75bbe13..2e7498a821a48 100644
@@ -49,7 +49,8 @@ int fscrypt_file_open(struct inode *inode, struct file *filp)
 }
 EXPORT_SYMBOL_GPL(fscrypt_file_open);
 
-int __fscrypt_prepare_link(struct inode *inode, struct inode *dir)
+int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
+			   struct dentry *dentry)
 {
 	int err;

@@ -57,6 +58,10 @@ int __fscrypt_prepare_link(struct inode *inode, struct inode *dir)
 	if (err)
 		return err;
 
+	/* ... in case we looked up ciphertext name before key was added */
+	if (dentry->d_flags & DCACHE_ENCRYPTED_NAME)
+		return -ENOKEY;
+
 	if (!fscrypt_has_permitted_context(dir, inode))
 		return -EXDEV;

@@ -78,6 +83,11 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (err)
 		return err;
 
+	/* ... in case we looked up ciphertext name(s) before key was added */
+	if ((old_dentry->d_flags | new_dentry->d_flags) &
+	    DCACHE_ENCRYPTED_NAME)
+		return -ENOKEY;
+
 	if (old_dir != new_dir) {
 		if (IS_ENCRYPTED(new_dir) &&
 		    !fscrypt_has_permitted_context(new_dir,

include/linux/fscrypt.h +5 -4

index 5a0c3fee1ea2b..4ad7a856e0f1c 100644
@@ -214,7 +214,8 @@ extern int fscrypt_zeroout_range(const struct inode *, pgoff_t, sector_t,
 
 /* hooks.c */
 extern int fscrypt_file_open(struct inode *inode, struct file *filp);
-extern int __fscrypt_prepare_link(struct inode *inode, struct inode *dir);
+extern int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
+				  struct dentry *dentry);
 extern int __fscrypt_prepare_rename(struct inode *old_dir,
 				    struct dentry *old_dentry,
 				    struct inode *new_dir,

@@ -401,8 +402,8 @@ static inline int fscrypt_file_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
-static inline int __fscrypt_prepare_link(struct inode *inode,
-					 struct inode *dir)
+static inline int __fscrypt_prepare_link(struct inode *inode, struct inode *dir,
+					 struct dentry *dentry)
 {
 	return -EOPNOTSUPP;
 }

@@ -497,7 +498,7 @@ static inline int fscrypt_prepare_link(struct dentry *old_dentry,
 				       struct dentry *dentry)
 {
 	if (IS_ENCRYPTED(dir))
-		return __fscrypt_prepare_link(d_inode(old_dentry), dir);
+		return __fscrypt_prepare_link(d_inode(old_dentry), dir, dentry);
 	return 0;
 }

Signature · 8 lines

2.21.0.225.g810b269d1ac-goog



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2019-04-17 13:53 UTC #6 lore raw

Re: [PATCH v2 2/5] fscrypt: fix race allowing rename() and link() of ciphertext dentries

Quoted 6 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Close some race conditions where fscrypt allowed rename() and link() on ciphertext dentries that had been looked up just prior to the key being concurrently added. It's better to return -ENOKEY in this case.

Quoted 7 lines

This avoids doing the nonsensical thing of encrypting the names a second time when searching for the actual on-disk dir entries. It also guarantees that DCACHE_ENCRYPTED_NAME dentries are never rename()d, so the dcache won't have support all possible combinations of moving DCACHE_ENCRYPTED_NAME around during __d_move().
Signed-off-by Eric Biggers <[email protected]>

Looks good, applied.

- Ted

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #7 lore raw

PATCH v2 3/5 fs, fscrypt: clear DCACHE_ENCRYPTED_NAME when unaliasing directory

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Make __d_move() clear DCACHE_ENCRYPTED_NAME on the source dentry. This is needed for when d_splice_alias() moves a directory's encrypted alias to its decrypted alias as a result of the encryption key being added.

Otherwise, the decrypted alias will incorrectly be invalidated on the next lookup, causing problems such as unmounting a mount the user just mount()ed there.

Note that we don't have to support arbitrary moves of this flag because fscrypt doesn't allow dentries with DCACHE_ENCRYPTED_NAME to be the source or target of a rename().

Fixes 28b4c263961c ("ext4 crypto: revalidate dentry after adding or removing the key")

Reported-by Sarthak Kukreti <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 fs/dcache.c             |  2 ++
 include/linux/fscrypt.h | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

fs/dcache.c +2 -0

index aac41adf47433..647e6ed426e20 100644
@@ -18,6 +18,7 @@
 #include <linux/string.h>
 #include <linux/mm.h>
 #include <linux/fs.h>
+#include <linux/fscrypt.h>
 #include <linux/fsnotify.h>
 #include <linux/slab.h>
 #include <linux/init.h>

@@ -2795,6 +2796,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
 	list_move(&dentry->d_child, &dentry->d_parent->d_subdirs);
 	__d_rehash(dentry);
 	fsnotify_update_flags(dentry);
+	fscrypt_handle_d_move(dentry);
 
 	write_seqcount_end(&target->d_seq);
 	write_seqcount_end(&dentry->d_seq);

include/linux/fscrypt.h +16 -0

index 4ad7a856e0f1c..39bd2619a3424 100644
@@ -88,6 +88,18 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 		inode->i_sb->s_cop->dummy_context(inode);
 }
 
+/*
+ * When d_splice_alias() moves a directory's encrypted alias to its decrypted
+ * alias as a result of the encryption key being added, DCACHE_ENCRYPTED_NAME
+ * must be cleared.  Note that we don't have to support arbitrary moves of this
+ * flag because fscrypt doesn't allow encrypted aliases to be the source or
+ * target of a rename().
+ */
+static inline void fscrypt_handle_d_move(struct dentry *dentry)
+{
+	dentry->d_flags &= ~DCACHE_ENCRYPTED_NAME;
+}
+
 /* crypto.c */
 extern void fscrypt_enqueue_decrypt_work(struct work_struct *);
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);

@@ -243,6 +255,10 @@ static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
 	return false;
 }
 
+static inline void fscrypt_handle_d_move(struct dentry *dentry)
+{
+}
+
 /* crypto.c */
 static inline void fscrypt_enqueue_decrypt_work(struct work_struct *work)
 {

Signature · 1 line

2.21.0.225.g810b269d1ac-goog

Theodore Ts'o

maintainer

2019-04-17 14:06 UTC #8 lore raw

Re: [PATCH v2 3/5] fs, fscrypt: clear DCACHE_ENCRYPTED_NAME when unaliasing directory

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Make __d_move() clear DCACHE_ENCRYPTED_NAME on the source dentry. This is needed for when d_splice_alias() moves a directory's encrypted alias to its decrypted alias as a result of the encryption key being added.
Otherwise, the decrypted alias will incorrectly be invalidated on the next lookup, causing problems such as unmounting a mount the user just mount()ed there.

Quoted 7 lines

Note that we don't have to support arbitrary moves of this flag because fscrypt doesn't allow dentries with DCACHE_ENCRYPTED_NAME to be the source or target of a rename().
Fixes 28b4c263961c ("ext4 crypto: revalidate dentry after adding or removing the key")
Reported-by Sarthak Kukreti <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good, applied.

- Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #9 lore raw

PATCH v2 4/5 fscrypt: only set dentry_operations on ciphertext dentries

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Plaintext dentries are always valid, so only set fscrypt_d_ops on ciphertext dentries.

Besides marginally improved performance, this allows overlayfs to use an fscrypt-encrypted upperdir, provided that all the following are true:

(1) The fscrypt encryption key is placed in the keyring before mounting overlayfs, and remains while the overlayfs is mounted.

(2) The overlayfs workdir uses the same encryption policy.

(3) No dentries for the ciphertext names of subdirectories have been created in the upperdir or workdir yet. (Since otherwise d_splice_alias() will reuse the old dentry with ->d_op set.)

One potential use case is using an ephemeral encryption key to encrypt all files created or changed by a container, so that they can be securely erased ("crypto-shredded") after the container stops.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/hooks.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/crypto/hooks.c +1 -2

index 2e7498a821a48..9d8910e86ee5d 100644
@@ -115,9 +115,8 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry)
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_ENCRYPTED_NAME;
 		spin_unlock(&dentry->d_lock);
+		d_set_d_op(dentry, &fscrypt_d_ops);
 	}
-
-	d_set_d_op(dentry, &fscrypt_d_ops);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);

Signature · 8 lines

2.21.0.225.g810b269d1ac-goog



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2019-04-17 14:07 UTC #10 lore raw

Re: [PATCH v2 4/5] fscrypt: only set dentry_operations on ciphertext dentries

Quoted 17 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Plaintext dentries are always valid, so only set fscrypt_d_ops on ciphertext dentries.
Besides marginally improved performance, this allows overlayfs to use an fscrypt-encrypted upperdir, provided that all the following are true:
(1) The fscrypt encryption key is placed in the keyring before mounting overlayfs, and remains while the overlayfs is mounted.
(2) The overlayfs workdir uses the same encryption policy.
(3) No dentries for the ciphertext names of subdirectories have been created in the upperdir or workdir yet. (Since otherwise d_splice_alias() will reuse the old dentry with ->d_op set.)

Quoted 5 lines

One potential use case is using an ephemeral encryption key to encrypt all files created or changed by a container, so that they can be securely erased ("crypto-shredded") after the container stops.
Signed-off-by Eric Biggers <[email protected]>

Looks good, applied.

- Ted

Eric Biggers

author maintainer

2019-03-20 18:39 UTC #11 lore raw

PATCH v2 5/5 fscrypt: fix race where ->lookup() marks plaintext dentry as ciphertext

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

->lookup() in an encrypted directory begins as follows:

1. fscrypt_prepare_lookup():
a. Try to load the directory's encryption key.
b. If the key is unavailable, mark the dentry as a ciphertext name
via d_flags.
2. fscrypt_setup_filename():
a. Try to load the directory's encryption key.
b. If the key is available, encrypt the name (treated as a plaintext
name) to get the on-disk name. Otherwise decode the name (treated as a ciphertext name) to get the on-disk name.

But if the key is concurrently added, it may be found at (2a) but not at (1a). In this case, the dentry will be wrongly marked as a ciphertext name even though it was actually treated as plaintext.

This will cause the dentry to be wrongly invalidated on the next lookup, potentially causing problems. For example, if the racy ->lookup() was part of sys_mount(), then the new mount will be detached when anything tries to access it. This is despite the mountpoint having a plaintext path, which should remain valid now that the key was added.

Of course, this is only possible if there's a userspace race. Still, the additional kernel-side race is confusing and unexpected.

Close the kernel-side race by changing fscrypt_prepare_lookup() to also set the on-disk filename (step 2b), consistent with the d_flags update.

Fixes 28b4c263961c ("ext4 crypto: revalidate dentry after adding or removing the key")

Signed-off-by Eric Biggers <[email protected]>

---
 fs/crypto/fname.c       |  1 +
 fs/crypto/hooks.c       | 11 +++---
 fs/ext4/ext4.h          | 62 +++++++++++++++++++++++++--------
 fs/ext4/namei.c         | 76 ++++++++++++++++++++++++++++-------------
 fs/f2fs/namei.c         | 17 +++++----
 fs/ubifs/dir.c          |  8 ++---
 include/linux/fscrypt.h | 30 ++++++++++------
 7 files changed, 139 insertions(+), 66 deletions(-)

fs/crypto/fname.c +1 -0

index 7ff40a73dbece..dea65c091f713 100644
@@ -356,6 +356,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	}
 	if (!lookup)
 		return -ENOKEY;
+	fname->is_ciphertext_name = true;
 
 	/*
 	 * We don't have the key and we are doing a lookup; decode the

fs/crypto/hooks.c +6 -5

index 9d8910e86ee5d..042d5b44f4ed9 100644
@@ -104,20 +104,21 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry,
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_rename);
 
-int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry)
+int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
+			     struct fscrypt_name *fname)
 {
-	int err = fscrypt_get_encryption_info(dir);
+	int err = fscrypt_setup_filename(dir, &dentry->d_name, 1, fname);
 
-	if (err)
+	if (err && err != -ENOENT)
 		return err;
 
-	if (!fscrypt_has_encryption_key(dir)) {
+	if (fname->is_ciphertext_name) {
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_ENCRYPTED_NAME;
 		spin_unlock(&dentry->d_lock);
 		d_set_d_op(dentry, &fscrypt_d_ops);
 	}
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);

fs/ext4/ext4.h +47 ~~-15~~

89 lines not on this page · raw message

fs/ext4/namei.c +52 ~~-24~~

138 lines not on this page · raw message

fs/f2fs/namei.c +10 -7

index f5e34e4670031..c3e8a901d47ac 100644
@@ -436,19 +436,23 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	nid_t ino = -1;
 	int err = 0;
 	unsigned int root_ino = F2FS_ROOT_INO(F2FS_I_SB(dir));
+	struct fscrypt_name fname;
 
 	trace_f2fs_lookup_start(dir, dentry, flags);
 
-	err = fscrypt_prepare_lookup(dir, dentry, flags);
-	if (err)
-		goto out;
-
 	if (dentry->d_name.len > F2FS_NAME_LEN) {
 		err = -ENAMETOOLONG;
 		goto out;
 	}
 
-	de = f2fs_find_entry(dir, &dentry->d_name, &page);
+	err = fscrypt_prepare_lookup(dir, dentry, &fname);
+	if (err == -ENOENT)
+		goto out_splice;
+	if (err)
+		goto out;
+	de = __f2fs_find_entry(dir, &fname, &page);
+	fscrypt_free_filename(&fname);
+
 	if (!de) {
 		if (IS_ERR(page)) {
 			err = PTR_ERR(page);

@@ -488,8 +492,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 	}
 out_splice:
 	new = d_splice_alias(inode, dentry);
-	if (IS_ERR(new))
-		err = PTR_ERR(new);
+	err = PTR_ERR_OR_ZERO(new);
 	trace_f2fs_lookup_end(dir, dentry, ino, err);
 	return new;
 out_iput:

fs/ubifs/dir.c +3 -5

index 5767b373a8ffb..b73de6d04fa3e 100644
@@ -220,11 +220,9 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 
 	dbg_gen("'%pd' in dir ino %lu", dentry, dir->i_ino);
 
-	err = fscrypt_prepare_lookup(dir, dentry, flags);
-	if (err)
-		return ERR_PTR(err);
-
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
+	err = fscrypt_prepare_lookup(dir, dentry, &nm);
+	if (err == -ENOENT)
+		return d_splice_alias(NULL, dentry);
 	if (err)
 		return ERR_PTR(err);

include/linux/fscrypt.h +20 ~~-10~~

index 39bd2619a3424..c00b764d6b8c9 100644
@@ -33,6 +33,7 @@ struct fscrypt_name {
 	u32 hash;
 	u32 minor_hash;
 	struct fscrypt_str crypto_buf;
+	bool is_ciphertext_name;
 };
 
 #define FSTR_INIT(n, l)		{ .name = n, .len = l }

@@ -233,7 +234,8 @@ extern int __fscrypt_prepare_rename(struct inode *old_dir,
 				    struct inode *new_dir,
 				    struct dentry *new_dentry,
 				    unsigned int flags);
-extern int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry);
+extern int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
+				    struct fscrypt_name *fname);
 extern int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
 				     unsigned int max_len,
 				     struct fscrypt_str *disk_link);

@@ -347,7 +349,7 @@ static inline int fscrypt_setup_filename(struct inode *dir,
 	if (IS_ENCRYPTED(dir))
 		return -EOPNOTSUPP;
 
-	memset(fname, 0, sizeof(struct fscrypt_name));
+	memset(fname, 0, sizeof(*fname));
 	fname->usr_fname = iname;
 	fname->disk_name.name = (unsigned char *)iname->name;
 	fname->disk_name.len = iname->len;

@@ -434,7 +436,8 @@ static inline int __fscrypt_prepare_rename(struct inode *old_dir,
 }
 
 static inline int __fscrypt_prepare_lookup(struct inode *dir,
-					   struct dentry *dentry)
+					   struct dentry *dentry,
+					   struct fscrypt_name *fname)
 {
 	return -EOPNOTSUPP;
 }

@@ -555,25 +558,32 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
  * fscrypt_prepare_lookup - prepare to lookup a name in a possibly-encrypted directory
  * @dir: directory being searched
  * @dentry: filename being looked up
- * @flags: lookup flags
+ * @fname: (output) the name to use to search the on-disk directory
  *
- * Prepare for ->lookup() in a directory which may be encrypted.  Lookups can be
- * done with or without the directory's encryption key; without the key,
+ * Prepare for ->lookup() in a directory which may be encrypted by determining
+ * the name that will actually be used to search the directory on-disk.  Lookups
+ * can be done with or without the directory's encryption key; without the key,
  * filenames are presented in encrypted form.  Therefore, we'll try to set up
  * the directory's encryption key, but even without it the lookup can continue.
  *
  * This also installs a custom ->d_revalidate() method which will invalidate the
  * dentry if it was created without the key and the key is later added.
  *
- * Return: 0 on success, -errno if a problem occurred while setting up the
- * encryption key
+ * Return: 0 on success; -ENOENT if key is unavailable but the filename isn't a
+ * correctly formed encoded ciphertext name, so a negative dentry should be
+ * created; or another -errno code.
  */
 static inline int fscrypt_prepare_lookup(struct inode *dir,
 					 struct dentry *dentry,
-					 unsigned int flags)
+					 struct fscrypt_name *fname)
 {
 	if (IS_ENCRYPTED(dir))
-		return __fscrypt_prepare_lookup(dir, dentry);
+		return __fscrypt_prepare_lookup(dir, dentry, fname);
+
+	memset(fname, 0, sizeof(*fname));
+	fname->usr_fname = &dentry->d_name;
+	fname->disk_name.name = (unsigned char *)dentry->d_name.name;
+	fname->disk_name.len = dentry->d_name.len;
 	return 0;
 }

Signature · 8 lines

2.21.0.225.g810b269d1ac-goog



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2019-04-17 14:24 UTC #12 lore raw

Re: [PATCH v2 5/5] fscrypt: fix race where ->lookup() marks plaintext dentry as ciphertext

Quoted 24 lines · Eric Biggers

From: Eric Biggers <[email protected]>
->lookup() in an encrypted directory begins as follows:
1. fscrypt_prepare_lookup():
a. Try to load the directory's encryption key.
b. If the key is unavailable, mark the dentry as a ciphertext name
via d_flags.
2. fscrypt_setup_filename():
a. Try to load the directory's encryption key.
b. If the key is available, encrypt the name (treated as a plaintext
name) to get the on-disk name. Otherwise decode the name (treated as a ciphertext name) to get the on-disk name.
But if the key is concurrently added, it may be found at (2a) but not at (1a). In this case, the dentry will be wrongly marked as a ciphertext name even though it was actually treated as plaintext.
This will cause the dentry to be wrongly invalidated on the next lookup, potentially causing problems. For example, if the racy ->lookup() was part of sys_mount(), then the new mount will be detached when anything tries to access it. This is despite the mountpoint having a plaintext path, which should remain valid now that the key was added.

Quoted 8 lines

Of course, this is only possible if there's a userspace race. Still, the additional kernel-side race is confusing and unexpected.
Close the kernel-side race by changing fscrypt_prepare_lookup() to also set the on-disk filename (step 2b), consistent with the d_flags update.
Fixes 28b4c263961c ("ext4 crypto: revalidate dentry after adding or removing the key")
Signed-off-by Eric Biggers <[email protected]>

Looks good, applied.

- Ted

Eric Biggers

author maintainer

2019-04-01 17:11 UTC #13 lore raw

In reply to Eric Biggers

Quoted 56 lines · Eric Biggers

From: Eric Biggers <[email protected]>
This patch series improves dentry revalidation in fscrypt.
To recap, fscrypt (aka ext4/f2fs/ubifs encryption) encrypts both file contents and file names in individual directory trees. A single filesystem can contain many encrypted directory trees using many different encryption keys. Major users of fscrypt require the ability to delete encrypted files when their encryption key is unavailable, e.g. when the system needs to delete a removed user's home directory or free up space from a logged-out user's cache directory.
Therefore fscrypt allows listing, looking up, and deleting files in encrypted directories via encoded ciphertext names, but only before the key is added. After the key is added, the ciphertext names are invalidated via ->d_revalidate() and plaintext names are shown instead.
fscrypt isn't a stacked filesystem, and it's explicitly for storage encryption, not OS-level access control. Thus, whether each directory inode has its key or not is a global state, not per-process. Also, the inode keeps its key until it's evicted from the inode cache. So, plaintext names shouldn't ever get invalidated by ->d_revalidate().
This patch series makes the following improvements:
- Only assign ->d_revalidate() to ciphertext filenames, thus allowing overlayfs to use an fscrypt-encrypted upperdir in some cases. (Previous discussion: https://lkml.org/lkml/2019/3/13/255)
- Fix cases where plaintext filenames would wrongly be invalidated, including a real-world bug recently reported on Chromium OS.
- Fix cases where ciphertext filenames would wrongly not be invalidated.
- Allow rcu-walk lookups in encrypted directories with the key, which improves performance.
(Previous attempt: https://patchwork.kernel.org/patch/10594133/)
- Fix cases where rename() and link() could succeed on ciphertext names.
Changed since v1:
- Fixed comment in fscrypt_d_revalidate() to explain that dget_parent() is actually still required.
- Moved clearing DCACHE_ENCRYPTED_NAME into fscrypt.h, to avoid an extra #ifdef and cluttering up dcache.c.
Eric Biggers (5):
fscrypt: clean up and improve dentry revalidation
fscrypt: fix race allowing rename() and link() of ciphertext dentries fs, fscrypt: clear DCACHE_ENCRYPTED_NAME when unaliasing directory
fscrypt: only set dentry_operations on ciphertext dentries
fscrypt: fix race where ->lookup() marks plaintext dentry as
ciphertext

Quoted 15 lines

 fs/crypto/crypto.c      | 58 ++++++++++++++++---------------
 fs/crypto/fname.c       |  1 +
 fs/crypto/hooks.c       | 28 ++++++++++-----
 fs/dcache.c             |  2 ++
 fs/ext4/ext4.h          | 62 +++++++++++++++++++++++++--------
 fs/ext4/namei.c         | 76 ++++++++++++++++++++++++++++-------------
 fs/f2fs/namei.c         | 17 +++++----
 fs/ubifs/dir.c          |  8 ++---
 include/linux/dcache.h  |  2 +-
 include/linux/fscrypt.h | 61 +++++++++++++++++++++++----------
 10 files changed, 208 insertions(+), 107 deletions(-)

Signature · 1 line

2.21.0.225.g810b269d1ac-goog

Any more comments on these patches? I'd like to apply them to the fscrypt tree for 5.2. Richard, can you check whether these solve your overlayfs use case? Also, patch 5 could use Acks from ext4, f2fs, and ubifs people.

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Richard Weinberger

maintainer

2019-04-08 08:22 UTC #14 lore raw

Am Montag, 1. April 2019, 19:11:21 CEST schrieb Eric Biggers:

Quoted 3 lines

Any more comments on these patches? I'd like to apply them to the fscrypt tree for 5.2. Richard, can you check whether these solve your overlayfs use case? Also, patch 5 could use Acks from ext4, f2fs, and ubifs people.

Will check this week, traveling right now.

Thanks,
//richard