On Wed, Mar 19, 2025 at 11:01:34AM +0000, Jakub Acs wrote:

If the rec_len of '.' is precisely one block (4KB), it slips through the sanity checks (it is considered the last directory entry in the data block) and leaves "struct ext4_dir_entry_2 *de" point exactly past the memory slot allocated to the data block. The following call to ext4_check_dir_entry() on new value of de then dereferences this pointer which results in out-of-bounds mem access.

Fix this by extending __ext4_check_dir_entry() to check for '.' dir entries that reach the end of data block. Make sure to ignore the phony dir entries for checksum (by checking name_len for non-zero).

Note: This is reported by KASAN as use-after-free in case another structure was recently freed from the slot past the bound, but it is really an OOB read.

Well, a (non-inline) directory where '.' is a single directory entry that fills the file system block should (probably) never occur in nature. So from that perspective, the check that you propose is probably fine.

HOWEVER. The check that e2fsck does is slightly different, which is stronger in some ways, and weaker than others relative to what you propose. What e2fsck checks for non-inline directory is that '.' must be first entry, and '..' must be the second directory entry.

So if somehow, a file system gets created where '.' is the first entry that fills the first directory block, and '..' is the second entry which is at the beginning of the second directory block, we would be in the interesting situation where it's possible that the kernel would report the file system to be corrupted --- not in the case of check_empty_dir() used in the rmdir_path, but there are other calls to check_dir_entry that will result in the file system to be declare corrupted --- then when the user runs e2fsck on a file system declared corrupted by the kernel, e2fsck would return a Big Thumbs Up; but then when the kernel trips over that directory again, it would get declared corrupted again.

So this ultimately turns on the definition of "valid". If the definition is "could this happen naturally, at least using all Linux implementations that I'm aware of", then no, it's not valid. However, if there is some other implementation of ext2/ext3/ext4 (say, BSD, Hurd, etc.), or a potentially malicious user (or a fuzzer) carefully crafts a file system, and e2fsck reports that the file system doesn't have any problems, then yes there _could_ be valid file systems where '.' is both the first and last directory entry.

Is it likely that there are other legitimate implementations that would create such a file system? No, it's highly unlikely. So one approach might be to make this change in what is officially considered valid as defined by e2fsck. The downside of this would be that there could be a version skew, where the kernel change gets backported into an LTS kernel, but the user doesn't upgrade to a newer version of e2fsprogs, the user might get confused.

In this particular case, I think it's worthwhile to make the change, since if we don't make the change in __ext4_check_dir_entry(), it's not enough to make a change in ext4_empty_dir(). We'd have to audit *all* of check_dir_entry's, and a quick check indicates we'd also need to add a check to ext4_get_first_dir_block().

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c index 02d47a64e8d1..d157a6c0eff6 100644 --- a/fs/ext4/dir.c +++ b/fs/ext4/dir.c @@ -104,6 +104,9 @@ int __ext4_check_dir_entry(const char *function, unsigned int line, else if (unlikely(le32_to_cpu(de->inode) > le32_to_cpu(EXT4_SB(dir->i_sb)->s_es->s_inodes_count))) error_msg = "inode out of bounds";

• else if (unlikely(de->name_len > 0 && strcmp(".", de->name) == 0 &&

• 	  next_offset == size))
  

• error_msg = "'.' directory cannot be the last in data block";
  

  else return 0;

I'd change the check to:

else if (unlikely(next_offset == size && de->name_len == 1 && strcmp(".", de->name) == 0))

which is a bit more optimized.

So if you resend this commit with this change, and remove the question to the ext4 maintainers (for future reference, it's best to put things that don't need to be in the commit if the patch gets accepted after the '---' line and before the diffstat, since that way if the maintainer is ready to accept the patch, they won't have the edit the commit description), I'd be happy to accept the patch.

Also, the best way to test a commit is not just to run all of the ext4/* xfstests patches, but to run a smoke test. If you use kvm-xfstests[1], you can do this via "kvm-xfstests smoke", which is syntactic sugar for "SOAK_DURATION=3m check -g smoketest" and will take 15-20 minutes. If you want to spend a bit more time, you can run the quick group ("check -g quick" or "kvm-xfstests -c ext4/4k -g quick").

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quicksta...

The reason why I mention using kvm-xfstests is because in the future, if you try your hand at fixing a syzbot issue that uses a slightly more exotic feature, such as inline_data, you'd want to do something like "kvm-xfstests -c ext4/inline <test_specifiers>" so that the file system is set up correctly for testing those code paths. And of course, for more testing fun, please see gce-xfstests[2].

[2] https://thunk.org/gce-xfstests

Many thanks,

- Ted

P.S. If anyone is interested in adding support for other cloud platforms such as Amazon and Azure as an additional back ends to xfstests-bld -- we have support for MacOS's hvf, Docker and Android as back ends, and more would be great; please contact me. Patches Gratefully Accepted. :-)