Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Gabriel Krisman Bertazi <krisman@suse.de>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-ext4@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
        linux-fsdevel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [PATCH 1/3] ext4: reject casefold inode flag without casefold
 feature
In-Reply-To: <20230814182903.37267-2-ebiggers@kernel.org> (Eric Biggers's
        message of "Mon, 14 Aug 2023 11:29:01 -0700")
Organization: SUSE
References: <20230814182903.37267-1-ebiggers@kernel.org>
        <20230814182903.37267-2-ebiggers@kernel.org>
Date:   Mon, 14 Aug 2023 15:09:33 -0400
Message-ID: <87jztx5tle.fsf@suse.de>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> writes:

> From: Eric Biggers <ebiggers@google.com>
>
> It is invalid for the casefold inode flag to be set without the casefold
> superblock feature flag also being set.  e2fsck already considers this
> case to be invalid and handles it by offering to clear the casefold flag
> on the inode.  __ext4_iget() also already considered this to be invalid,
> sort of, but it only got so far as logging an error message; it didn't
> actually reject the inode.  Make it reject the inode so that other code
> doesn't have to handle this case.  This matches what f2fs does.
>
> Note: we could check 's_encoding != NULL' instead of
> ext4_has_feature_casefold().  This would make the check robust against
> the casefold feature being enabled by userspace writing to the page
> cache of the mounted block device.  However, it's unsolvable in general
> for filesystems to be robust against concurrent writes to the page cache
> of the mounted block device.  Though this very particular scenario
> involving the casefold feature is solvable, we should not pretend that
> we can support this model, so let's just check the casefold feature.
> tune2fs already forbids enabling casefold on a mounted filesystem.

just because we can't fix the general issue for the entire filesystem
doesn't mean this case *must not* ever be addressed. What is the
advantage of making the code less robust against the syzbot code?  Just
check sb->s_encoding and be safe later knowing the unicode map is
available.

>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/ext4/inode.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 43775a6ca505..390dedbb7e8a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4940,9 +4940,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  				 "iget: bogus i_mode (%o)", inode->i_mode);
>  		goto bad_inode;
>  	}
> -	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb))
> +	if (IS_CASEFOLDED(inode) && !ext4_has_feature_casefold(inode->i_sb)) {
>  		ext4_error_inode(inode, function, line, 0,
>  				 "casefold flag without casefold feature");
> +		ret = -EFSCORRUPTED;
> +		goto bad_inode;
> +	}
>  	if ((err_str = check_igot_inode(inode, flags)) != NULL) {
>  		ext4_error_inode(inode, function, line, 0, err_str);
>  		ret = -EFSCORRUPTED;

-- 
Gabriel Krisman Bertazi