| Message ID | 20230814182903.37267-2-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8216776ccff6fcd40e3fdaa109aa4150ebe760b3 |
| Headers | show |
| Series | Simplify rejection of unexpected casefold inode flag | expand |
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.
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;