Received-SPF: pass (google.com: domain of linux-ext4+bounces-547-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Gabriel Krisman Bertazi <krisman@suse.de>
To: Eric Biggers <ebiggers@kernel.org>, Amir Goldstein <amir73il@gmail.com>
Cc: viro@zeniv.linux.org.uk,  jaegeuk@kernel.org,  tytso@mit.edu,
  linux-f2fs-devel@lists.sourceforge.net,  linux-ext4@vger.kernel.org,
  linux-fsdevel@vger.kernel.org
Subject: [PATCH] ovl: Reject mounting case-insensitive filesystems
In-Reply-To: <20231219231222.GI38652@quark.localdomain> (Eric Biggers's
	message of "Tue, 19 Dec 2023 16:12:22 -0700")
Organization: SUSE
References: <20231215211608.6449-1-krisman@suse.de>
	<20231219231222.GI38652@quark.localdomain>
Date: Fri, 22 Dec 2023 23:23:41 -0500
Message-ID: <87a5q1eecy.fsf_-_@mailhost.krisman.be>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Eric Biggers <ebiggers@kernel.org> writes:

> On Fri, Dec 15, 2023 at 04:16:00PM -0500, Gabriel Krisman Bertazi wrote:
>> [Apologies for the quick spin of a v2.  The only difference are a couple
>> fixes to the build when CONFIG_UNICODE=n caught by LKP and detailed in
>> each patch changelog.]
>> 
>> When case-insensitive and fscrypt were adapted to work together, we moved the
>> code that sets the dentry operations for case-insensitive dentries(d_hash and
>> d_compare) to happen from a helper inside ->lookup.  This is because fscrypt
>> wants to set d_revalidate only on some dentries, so it does it only for them in
>> d_revalidate.
>> 
>> But, case-insensitive hooks are actually set on all dentries in the filesystem,
>> so the natural place to do it is through s_d_op and let d_alloc handle it [1].
>> In addition, doing it inside the ->lookup is a problem for case-insensitive
>> dentries that are not created through ->lookup, like those coming
>> open-by-fhandle[2], which will not see the required d_ops.
>> 
>> This patchset therefore reverts to using sb->s_d_op to set the dentry operations
>> for case-insensitive filesystems.  In order to set case-insensitive hooks early
>> and not require every dentry to have d_revalidate in case-insensitive
>> filesystems, it introduces a patch suggested by Al Viro to disable d_revalidate
>> on some dentries on the fly.
>> 
>> It survives fstests encrypt and quick groups without regressions.  Based on
>> v6.7-rc1.
>> 
>> [1] https://lore.kernel.org/linux-fsdevel/20231123195327.GP38156@ZenIV/
>> [2] https://lore.kernel.org/linux-fsdevel/20231123171255.GN38156@ZenIV/
>> 
>> Gabriel Krisman Bertazi (8):
>>   dcache: Add helper to disable d_revalidate for a specific dentry
>>   fscrypt: Drop d_revalidate if key is available
>>   libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
>>   libfs: Expose generic_ci_dentry_ops outside of libfs
>>   ext4: Set the case-insensitive dentry operations through ->s_d_op
>>   f2fs: Set the case-insensitive dentry operations through ->s_d_op
>>   libfs: Don't support setting casefold operations during lookup
>>   fscrypt: Move d_revalidate configuration back into fscrypt
>
> Thanks Gabriel, this series looks good.  Sorry that we missed this when adding
> the support for encrypt+casefold.
>
> It's slightly awkward that some lines of code added by patches 5-6 are removed
> in patch 8.  These changes look very hard to split up, though, so you've
> probably done about the best that can be done.
>
> One question/request: besides performance, the other reason we're so careful
> about minimizing when ->d_revalidate is set for fscrypt is so that overlayfs
> works on encrypted directories.  This is because overlayfs is not compatible
> with ->d_revalidate.  I think your solution still works for that, since
> DCACHE_OP_REVALIDATE will be cleared after the first call to
> fscrypt_d_revalidate(), and when checking for usupported dentries overlayfs does
> indeed check for DCACHE_OP_REVALIDATE instead of ->d_revalidate directly.
> However, that does rely on that very first call to ->d_revalidate actually
> happening before the check is done.  It would be nice to verify that
> overlayfs+fscrypt indeed continues to work, and explicitly mention this
> somewhere (I don't see any mention of overlayfs+fscrypt in the series).

Hi Eric,

From my testing, overlayfs+fscrypt should work fine.  I tried mounting
it on top of encrypted directories, with and without key, and will add
this information to the commit message.  If we adopt the suggestion from
Al in the other subthread, even better, we won't need the first
d_revalidate to happen before the check, so I'll adopt that.

While looking into overlayfs, I found another reason we would need this
patchset.  A side effect of not configuring ->d_op through s_d_op is
that the root dentry won't have d_op set.  While this is fine for
casefold, because we forbid the root directory from being
case-insensitive, we can trick overlayfs into mounting a
filesystem it can't handle.

Even with this merged, and as Christian said in another thread regarding
ecryptfs, we should handle this explicitly.  Something like below.

Amir, would you consider this for -rc8?

-- >8 --
Subject: [PATCH] ovl: Reject mounting case-insensitive filesystems

overlayfs relies on the filesystem setting DCACHE_OP_HASH or
DCACHE_OP_COMPARE to reject mounting over case-insensitive directories.

Since commit bb9cd9106b22 ("fscrypt: Have filesystems handle their
d_ops"), we set ->d_op through a hook in ->d_lookup, which
means the root dentry won't have them, causing the mount to accidentally
succeed.

In v6.7-rc7, the following sequence will succeed to mount, but any
dentry other than the root dentry will be a "weird" dentry to ovl and
fail with EREMOTE.

  mkfs.ext4 -O casefold lower.img
  mount -O loop lower.img lower
  mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work ovl /mnt

Mounting on a subdirectory fails, as expected, because DCACHE_OP_HASH
and DCACHE_OP_COMPARE are properly set by ->lookup.

Fix by explicitly rejecting superblocks that allow case-insensitive
dentries.

Fixes: bb9cd9106b22 ("fscrypt: Have filesystems handle their d_ops")
Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/overlayfs/params.c | 2 ++
 include/linux/fs.h    | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 3fe2dde1598f..99495f079644 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -286,6 +286,8 @@ static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
 	if (!d_is_dir(path->dentry))
 		return invalfc(fc, "%s is not a directory", name);
 
+	if (sb_has_encoding(path->mnt->mnt_sb))
+		return invalfc(fc, "caseless filesystem on %s not supported", name);
 
 	/*
 	 * Check whether upper path is read-only here to report failures
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..e6667ece5e64 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3203,6 +3203,15 @@ extern int generic_check_addressable(unsigned, u64);
 
 extern void generic_set_encrypted_ci_d_ops(struct dentry *dentry);
 
+static inline bool sb_has_encoding(const struct super_block *sb)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	return !!sb->s_encoding;
+#else
+	return false;
+#endif
+}
+
 int may_setattr(struct mnt_idmap *idmap, struct inode *inode,
 		unsigned int ia_valid);
 int setattr_prepare(struct mnt_idmap *, struct dentry *, struct iattr *);
-- 
2.43.0