| Message ID | 20220514175929.44439-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | f2fs: don't use casefolded comparison for "." and ".." | expand |
Eric Biggers <ebiggers@kernel.org> writes: > From: Eric Biggers <ebiggers@google.com> > > 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: <stable@vger.kernel.org> # v5.11+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > 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 <krisman@collabora.com> Thanks,
Thanks, applied. On 05/16, Gabriel Krisman Bertazi wrote: > Eric Biggers <ebiggers@kernel.org> writes: > > > From: Eric Biggers <ebiggers@google.com> > > > > 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: <stable@vger.kernel.org> # v5.11+ > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > 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 <krisman@collabora.com> > > Thanks, > > -- > Gabriel Krisman Bertazi > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index a0e51937d92eb..d5bd7932fb642 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -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) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 492af5b96de19..e9e32bc814dfe 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -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 diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c index 3cb1e7a24740f..049ce50cec9b0 100644 --- a/fs/f2fs/hash.c +++ b/fs/f2fs/hash.c @@ -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) {