Skip to contentSkip to conversation

f2fs: don't use casefolded comparison for "." and ".."

PATCH Applied Eric Biggers 3 participants 3 messages May 14, 2022 – May 16, 2022

Subsystems (3)
Stable branch F2FS file system fscrypt: file system level encryption support
lore
Share & tools
lore

Eric Biggers

author maintainer
#0 lore raw

PATCH 1/1 f2fs: don't use casefolded comparison for "." and ".."

From: Eric Biggers <[email protected]>

Tryng to rename a directory that has all following properties fails with EINVAL and triggers the 'WARN_ON_ONCE(!fscrypt_has_encryption_key(dir))' in f2fs_match_ci_name():

- The directory is casefolded
- The directory is encrypted
- The directory's encryption key is not yet set up
- The parent directory is *not* encrypted

The problem is incorrect handling of the lookup of ".." to get the parent reference to update. fscrypt_setup_filename() treats ".." (and ".") specially, as it's never encrypted. It's passed through as-is, and setting up the directory's key is not attempted. As the name isn't a no-key name, f2fs treats it as a "normal" name and attempts a casefolded comparison. That breaks the assumption of the WARN_ON_ONCE() in f2fs_match_ci_name() which assumes that for encrypted directories, casefolded comparisons only happen when the directory's key is set up.

We could just remove this WARN_ON_ONCE(). However, since casefolding is always a no-op on "." and ".." anyway, let's instead just not casefold these names. This results in the standard bytewise comparison.

Fixes 7ad08a58bf67 ("f2fs: Handle casefolding with Encryption")
Cc <[email protected]> # v5.11+
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/dir.c  |  3 ++-
 fs/f2fs/f2fs.h | 10 +++++-----
 fs/f2fs/hash.c | 11 ++++++-----
 3 files changed, 13 insertions(+), 11 deletions(-)
fs/f2fs/dir.c +2 -1
index a0e51937d92eb..d5bd7932fb642 100644
@@ -82,7 +82,8 @@ int f2fs_init_casefolded_name(const struct inode *dir,
 #if IS_ENABLED(CONFIG_UNICODE)
 	struct super_block *sb = dir->i_sb;
 
-	if (IS_CASEFOLDED(dir)) {
+	if (IS_CASEFOLDED(dir) &&
+	    !is_dot_dotdot(fname->usr_fname->name, fname->usr_fname->len)) {
 		fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
 					GFP_NOFS, false, F2FS_SB(sb));
 		if (!fname->cf_name.name)
fs/f2fs/f2fs.h +5 -5
index 492af5b96de19..e9e32bc814dfe 100644
@@ -508,11 +508,11 @@ struct f2fs_filename {
 #if IS_ENABLED(CONFIG_UNICODE)
 	/*
 	 * For casefolded directories: the casefolded name, but it's left NULL
-	 * if the original name is not valid Unicode, if the directory is both
-	 * casefolded and encrypted and its encryption key is unavailable, or if
-	 * the filesystem is doing an internal operation where usr_fname is also
-	 * NULL.  In all these cases we fall back to treating the name as an
-	 * opaque byte sequence.
+	 * if the original name is not valid Unicode, if the original name is
+	 * "." or "..", if the directory is both casefolded and encrypted and
+	 * its encryption key is unavailable, or if the filesystem is doing an
+	 * internal operation where usr_fname is also NULL.  In all these cases
+	 * we fall back to treating the name as an opaque byte sequence.
 	 */
 	struct fscrypt_str cf_name;
 #endif
fs/f2fs/hash.c +6 -5
index 3cb1e7a24740f..049ce50cec9b0 100644
@@ -91,7 +91,7 @@ static u32 TEA_hash_name(const u8 *p, size_t len)
 /*
  * Compute @fname->hash.  For all directories, @fname->disk_name must be set.
  * For casefolded directories, @fname->usr_fname must be set, and also
- * @fname->cf_name if the filename is valid Unicode.
+ * @fname->cf_name if the filename is valid Unicode and is not "." or "..".
  */
 void f2fs_hash_filename(const struct inode *dir, struct f2fs_filename *fname)
 {

@@ -110,10 +110,11 @@ void f2fs_hash_filename(const struct inode *dir, struct f2fs_filename *fname)
 		/*
 		 * If the casefolded name is provided, hash it instead of the
 		 * on-disk name.  If the casefolded name is *not* provided, that
-		 * should only be because the name wasn't valid Unicode, so fall
-		 * back to treating the name as an opaque byte sequence.  Note
-		 * that to handle encrypted directories, the fallback must use
-		 * usr_fname (plaintext) rather than disk_name (ciphertext).
+		 * should only be because the name wasn't valid Unicode or was
+		 * "." or "..", so fall back to treating the name as an opaque
+		 * byte sequence.  Note that to handle encrypted directories,
+		 * the fallback must use usr_fname (plaintext) rather than
+		 * disk_name (ciphertext).
 		 */
 		WARN_ON_ONCE(!fname->usr_fname->name);
 		if (fname->cf_name.name) {

base-commit: c0d31ec6397d963d85a190996b4b74654ef32e1d

Signature · 1 line
2.36.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 24 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Tryng to rename a directory that has all following properties fails with EINVAL and triggers the 'WARN_ON_ONCE(!fscrypt_has_encryption_key(dir))' in f2fs_match_ci_name():

- The directory is casefolded
- The directory is encrypted
- The directory's encryption key is not yet set up
- The parent directory is *not* encrypted

The problem is incorrect handling of the lookup of ".." to get the parent reference to update. fscrypt_setup_filename() treats ".." (and ".") specially, as it's never encrypted. It's passed through as-is, and setting up the directory's key is not attempted. As the name isn't a no-key name, f2fs treats it as a "normal" name and attempts a casefolded comparison. That breaks the assumption of the WARN_ON_ONCE() in f2fs_match_ci_name() which assumes that for encrypted directories, casefolded comparisons only happen when the directory's key is set up.

We could just remove this WARN_ON_ONCE(). However, since casefolding is always a no-op on "." and ".." anyway, let's instead just not casefold these names. This results in the standard bytewise comparison.

Quoted 8 lines
Fixes 7ad08a58bf67 ("f2fs: Handle casefolding with Encryption")
Cc <[email protected]> # v5.11+
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/dir.c  |  3 ++-
 fs/f2fs/f2fs.h | 10 +++++-----
 fs/f2fs/hash.c | 11 ++++++-----
 3 files changed, 13 insertions(+), 11 deletions(-)

Hi Eric,

This looks good to me:

Reviewed-by Gabriel Krisman Bertazi <[email protected]>

Thanks,

Signature · 1 line
Gabriel Krisman Bertazi
maintainer Applied
#2 lore raw

Thanks, applied.

Quoted 51 lines · Gabriel Krisman Bertazi
Quoted 24 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Tryng to rename a directory that has all following properties fails with EINVAL and triggers the 'WARN_ON_ONCE(!fscrypt_has_encryption_key(dir))' in f2fs_match_ci_name():

- The directory is casefolded
- The directory is encrypted
- The directory's encryption key is not yet set up
- The parent directory is *not* encrypted

The problem is incorrect handling of the lookup of ".." to get the parent reference to update. fscrypt_setup_filename() treats ".." (and ".") specially, as it's never encrypted. It's passed through as-is, and setting up the directory's key is not attempted. As the name isn't a no-key name, f2fs treats it as a "normal" name and attempts a casefolded comparison. That breaks the assumption of the WARN_ON_ONCE() in f2fs_match_ci_name() which assumes that for encrypted directories, casefolded comparisons only happen when the directory's key is set up.

We could just remove this WARN_ON_ONCE(). However, since casefolding is always a no-op on "." and ".." anyway, let's instead just not casefold these names. This results in the standard bytewise comparison.

Quoted 8 lines
Fixes 7ad08a58bf67 ("f2fs: Handle casefolding with Encryption")
Cc <[email protected]> # v5.11+
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/dir.c  |  3 ++-
 fs/f2fs/f2fs.h | 10 +++++-----
 fs/f2fs/hash.c | 11 ++++++-----
 3 files changed, 13 insertions(+), 11 deletions(-)

Hi Eric,

This looks good to me:

Reviewed-by Gabriel Krisman Bertazi <[email protected]>

Thanks,

Signature · 7 lines
Gabriel Krisman Bertazi


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