linux-ext4 - Re: [PATCH 1/3] ext4: reject casefold inode flag without casefold feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2023 12:24:06 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Gabriel Krisman Bertazi <krisman@...e.de>
Cc:     linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [PATCH 1/3] ext4: reject casefold inode flag without casefold
 feature

On Mon, Aug 14, 2023 at 03:09:33PM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@...nel.org> writes:
> 
> > From: Eric Biggers <ebiggers@...gle.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