[1/3] ext4: reject casefold inode flag without casefold feature

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.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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;

On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote:
> 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.
> 

Just to make sure, it sounds like you agree that the late checks of ->s_encoding
are not needed and only __ext4_iget() should handle it, right?  That simplifies
the code so it is obviously beneficial if we can do it.

As for whether __ext4_iget() should check the casefold feature or ->s_encoding,
we should simply go with the one that makes the code clearer, as per what I've
said.  I think it's casefold, but it could be ->s_encoding if others prefer.

- Eric

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote:
>> 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.
>> 
>
> Just to make sure, it sounds like you agree that the late checks of ->s_encoding
> are not needed and only __ext4_iget() should handle it, right?  That simplifies
> the code so it is obviously beneficial if we can do it.

Yes.  After we get the inode from __ext4_iget, I think it doesn't matter
if the user went behind our back straight to the block device and
changed the superblock to remove the feature bit. If we already loaded
->s_encoding, it won't be unloaded, so only checking at ext4_iget should
be enough, as far as I can tell.