Re: [PATCH 27/27] ext4: add KUnit tests for fs/ext4/dir.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ext4: uninitialized i_size_seqcount in dir_test_init() on 32-bit
  SMP
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ext4: out-of-bounds read in ext4_readdir() on fake dirents
  [1]
--

commit 789413bd80e96edcb89e48a8973879c9137951c1
Author: Theodore Ts'o <tytso@mit.edu>

ext4: add KUnit tests for fs/ext4/dir.c

This commit adds a KUnit test suite for fs/ext4/dir.c covering directory
entry validation, hash-cookie helpers, directory llseek, and linear and
HTree directory iteration, and exports helper symbols for testing.

> diff --git a/fs/ext4/dir-test.c b/fs/ext4/dir-test.c
> new file mode 100644
> index 0000000000000..4d7c7c89e8b90
> --- /dev/null
> +++ b/fs/ext4/dir-test.c

[ ... ]

> +static int dir_test_init(struct kunit *test)
> +{
> +	struct dir_test_ctx *ctx;
> +	struct inode *dir;
> +	int i;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +	test->priv = ctx;

[ ... ]

> +	dir = &ctx->dir_ei.vfs_inode;
> +	dir->i_sb = &ctx->sb;
> +	dir->i_ino = DIR_TEST_DIR_INO;
> +	dir->i_mode = S_IFDIR | 0755;
> +	dir->i_blkbits = DIR_TEST_BLK_BITS;
> +	dir->i_mapping = &dir->i_data;
> +	dir->i_data.host = dir;
> +	set_nlink(dir, 2);\n> +	atomic_set(&dir->i_count, 1);
> +	spin_lock_init(&dir->i_lock);
> +	init_rwsem(&dir->i_rwsem);
> +	INIT_LIST_HEAD(&dir->i_io_list);
> +	INIT_LIST_HEAD(&dir->i_wb_list);
> +	INIT_LIST_HEAD(&dir->i_lru);
> +	INIT_HLIST_NODE(&dir->i_hash);
> +	inode_state_set_raw(dir, I_DIRTY);
> +	inode_set_iversion_raw(dir, 10);

[Severity: Low]
Does dir_test_init() leave dir->i_size_seqcount uninitialized on 32-bit SMP
builds?

Because ctx is allocated on the heap via kunit_kzalloc() without calling
i_size_ordered_init(dir) (or inode_init_once(dir)), i_size_write(dir, ...)
and i_size_read(dir) in the test cases pass an uninitialized lockdep_map to
write_seqcount_begin() and read_seqcount_begin() when CONFIG_DEBUG_LOCK_ALLOC
is enabled, triggering a non-static key warning and disabling lockdep.

> +
> +	init_rwsem(&ctx->dir_ei.xattr_sem);

[ ... ]

> +static void test_check_dir_entry_and_all_de(struct kunit *test)
> +{

[ ... ]

> +	/*
> +	 * 4. Casefolded + encrypted directory requires space for
> +	 * ext4_dir_entry_hash (+8 bytes) on regular entries, while fake
> +	 * entries ('.', '..', csum tail) remain 12 bytes.
> +	 */
> +	dir->i_flags |= S_CASEFOLD | S_ENCRYPTED;
> +	write_de(buf, 24, 15, 16, EXT4_FT_REG_FILE, "ab");
> +	de = (struct ext4_dir_entry_2 *)(buf + 24);
> +	KUNIT_EXPECT_EQ(test, __ext4_check_dir_entry(__func__, __LINE__, dir,
> +						     NULL, de, bh, buf,
> +						     DIR_TEST_BLK_SIZE, 24), 1);

[Severity: Critical]
This problem wasn't introduced by this patch, but can ext4_readdir() perform
an 8-byte out-of-bounds read past the end of the directory block buffer on
fake entries [1]?

In encrypted and casefolded directories, __ext4_check_dir_entry() passes
NULL instead of dir to ext4_dir_rec_len() when is_fake_dir_entry(de) is
true, so it does not require space for the 8-byte ext4_dir_entry_hash:

fs/ext4/dir.c:__ext4_check_dir_entry() {
	...
	bool fake = is_fake_dir_entry(de);
	...
	if (unlikely(rlen < ext4_dir_rec_len(1, fake ? NULL : dir)))
		error_msg = "rec_len is smaller than minimal";
	else if (unlikely(rlen % 4 != 0))
		error_msg = "rec_len % 4 != 0";
	else if (unlikely(rlen < ext4_dir_rec_len(de->name_len,
							fake ? NULL : dir)))
		error_msg = "rec_len is too small for name_len";
	...
}

However, ext4_readdir() only checks that de->inode is non-zero before
reading EXT4_DIRENT_HASH(de) and EXT4_DIRENT_MINOR_HASH(de):

fs/ext4/dir.c:ext4_readdir() {
	...
			if (le32_to_cpu(de->inode)) {
				if (!IS_ENCRYPTED(inode)) {
					...
				} else {
					...
					if (IS_CASEFOLDED(inode)) {
						hash = EXT4_DIRENT_HASH(de);
						minor_hash = EXT4_DIRENT_MINOR_HASH(de);
					}
	...
}

If a corrupted directory places a fake entry (such as "." or "..", or an
entry with file_type set to EXT4_FT_DIR_CSUM and a non-zero de->inode) at
the end of the block with rlen sized without the 8-byte hash, reading the
hash accesses 8 bytes past bh->b_data + sb->s_blocksize, which can fault on
an unmapped page or leak adjacent memory to userspace via nokey_name.dirhash.

[1] https://sashiko.dev/#/bug/linux-7bcf348c-5fde-4625-bb87-69ecef239378

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112040638.1576836-1-tytso@mit.edu?part=27