Re: [PATCH v14 08/12] ext4: rename ext4_dir_rec_len() and clarify dirdata usage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] ext4: unsigned len underflow in make_indexed_dir() when dot_de has dirdata
- [High] ext4: __ext4_check_dir_entry() misses out-of-bounds dirdata before rec_len grows
- [High] ext4: add_dirent_to_buf() error return after do_split() leaves bh undirtied
--

commit 06e0b22cf991d67d7b351c0723ad5474e253ef41
Author: Artem Blagodarenko <artem.blagodarenko@gmail.com>

ext4: rename ext4_dir_rec_len() and clarify dirdata usage

This commit renames ext4_dir_rec_len() to ext4_dirent_rec_len(), updates
directory entry length calculations to use ext4_dir_entry_len() so dirdata
is accounted for, and adds a room check in add_dirent_to_buf() for split
blocks.

> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 8d7b81e6948e7..38911ae2bcfc0 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -89,16 +89,15 @@ int __ext4_check_dir_entry(const char *function, unsigned int line,
>  	bool fake = is_fake_dir_entry(de);
>  	bool has_csum = ext4_has_feature_metadata_csum(dir->i_sb);
>  
> -	if (unlikely(rlen < ext4_dir_rec_len(1, fake ? NULL : dir)))
> +	if (unlikely(rlen < ext4_dirent_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";
>  	else if (unlikely(next_offset > size))
>  		error_msg = "directory entry overrun";
> -	else if (unlikely(next_offset > size - ext4_dir_rec_len(1,
> +	else if (unlikely(rlen < ext4_dir_entry_len(de, size, fake ? NULL : dir)))
> +		error_msg = "rec_len is too small for name_len";

[Severity: High]
As reported by Sashiko on v13 [1], can __ext4_check_dir_entry() ever reject
an entry whose on-disk rec_len is too small for its dirdata here, or does a
later rec_len expansion cause stale buffer bytes to be annexed into the
entry?

In ext4_dir_entry_len(), ext4_dirent_get_data_len(de, rec_len) validates
each dirdata header against the entry's current rec_len and returns 0 when
offset >= rec_len, offset + sizeof(*ddh) > rec_len, or ddh_len > rec_len -
offset:

fs/ext4/ext4.h:ext4_dirent_get_data_len() {
    ...
	if (offset >= rec_len)
		return 0;
    ...
		if (extra_data_flags & 1) {
			if (offset + sizeof(*ddh) > rec_len)
				return 0;
			{
				u8 ddh_len = READ_ONCE(ddh->ddh_length);

				if (ddh_len == 0 || ddh_len > rec_len - offset)
					return 0;
				dlen += ddh_len + (dlen == 0);
				offset += ddh_len;
    ...
}

Because rlen % 4 != 0 is already checked earlier in
__ext4_check_dir_entry(), any non-zero dirdata returned by
ext4_dirent_get_data_len() satisfies 8 + de->name_len + dirdata == offset
<= rlen, so ext4_dirent_rec_len(de->name_len + dirdata, NULL) <= rlen.
When a corrupted entry has dirdata flags set with ddh_length exceeding its
current rec_len, ext4_dirent_get_data_len() returns 0 and
__ext4_check_dir_entry() accepts the entry.

Later, during do_split(), dx_move_dirents() and dx_pack_dirents() compute
rec_len = ext4_dir_entry_len(de, blocksize, dir) while de->rec_len still
has its smaller pre-split value, copying only those bytes before do_split()
expands the last entry's rec_len to the end of the half-block. Once
de->rec_len grows (either in do_split() or after
ext4_generic_delete_entry()), subsequent calls to ext4_dir_entry_len() in
add_dirent_to_buf() and ext4_insert_dentry() see the larger rec_len, so the
previously out-of-bounds ddh_length now satisfies ddh_len <= rec_len -
offset and annexes uncopied or stale buffer bytes into the directory entry.

[ ... ]

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 9953583ad6c3f..0c91e4da00fbc 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c

[ ... ]

> @@ -2122,10 +2122,11 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>  int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  		      void *buf, int buf_size,
>  		      struct ext4_filename *fname,
> -		      struct ext4_dir_entry_2 **dest_de)
> +		      struct ext4_dir_entry_2 **dest_de,
> +		      int dlen)
>  {
>  	struct ext4_dir_entry_2 *de;
> -	unsigned short reclen = ext4_dir_rec_len(fname_len(fname), dir);
> +	unsigned short reclen = ext4_dirent_rec_len(fname_len(fname) + dlen, dir);
>  	int nlen, rlen;
>  	unsigned int offset = 0;
>  	char *top;
> @@ -2138,7 +2139,7 @@ int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  			return -EFSCORRUPTED;
>  		if (ext4_match(dir, fname, de))
>  			return -EEXIST;
> -		nlen = ext4_dir_rec_len(de->name_len, dir);
> +		nlen = ext4_dir_entry_len(de, buf_size, dir);
>  		rlen = ext4_rec_len_from_disk(de->rec_len, buf_size);
>  		if ((de->inode ? rlen - nlen : rlen) >= reclen)
>  			break;

[Severity: Critical]
Can changing nlen to ext4_dir_entry_len() in ext4_find_dest_de() allow
unsigned len in make_indexed_dir() to underflow and overflow data2 in
memcpy(data2, de, len)?

In __ext4_add_entry() -> add_dirent_to_buf() -> ext4_find_dest_de() on
block 0 of an unindexed directory (where buf_size = blocksize - csum_size),
the loop runs while (char *)de <= buf + buf_size - reclen. Because nlen now
includes dirdata via ext4_dir_entry_len(), the '.' entry (dot_de) on a
corrupted or crafted image can have rlen > buf_size - reclen while its
dirdata makes rlen - nlen < reclen. In that case, ext4_find_dest_de()
checks dot_de, advances de to dotdot_de at an offset past top, exits the
loop without running ext4_check_dir_entry() on dotdot_de, and returns
-ENOSPC.

__ext4_add_entry() then calls make_indexed_dir(), which validates dotdot_de
through ext4_check_dx_root() and dx_get_dx_info(). However, both check the
end of dotdot_de against blocksize rather than blocksize - csum_size:

fs/ext4/namei.c:ext4_check_dx_root() {
    ...
	de = ext4_next_entry(dotdot_de, blocksize);
	if ((char *)de >= (((char *)dot_de) + blocksize)) {
		error_msg = "invalid rec_len for '..'";
		goto corrupted;
	}
    ...
}

When metadata_csum is enabled (csum_size = 12) and dotdot_de ends between
blocksize - csum_size + 1 and blocksize - 1, make_indexed_dir() computes:

fs/ext4/namei.c:make_indexed_dir() {
    ...
	/* The 0th block becomes the root, move the dirents out */
	len = ((char *)dot_de) + (blocksize - csum_size) - (char *)de;
    ...
	memcpy(data2, de, len);
}

Since (char *)de > (char *)dot_de + (blocksize - csum_size), unsigned len
underflows and is passed to memcpy(data2, de, len) and memset(de, 0, len).

[ ... ]

> @@ -2206,9 +2207,27 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
>  
>  	if (!de) {
>  		err = ext4_find_dest_de(dir, bh, bh->b_data,
> -					blocksize - csum_size, fname, &de);
> +					blocksize - csum_size, fname, &de, 0);
>  		if (err)
>  			return err;
> +	} else {
> +		/*
> +		 * do_split() hands over the last entry of a half.  Its length
> +		 * comes from on-disk dirdata, so check that the new entry fits
> +		 * before anything is written.
> +		 */
> +		unsigned int nlen = de->inode ?
> +			ext4_dir_entry_len(de, blocksize, dir) : 0;
> +		unsigned int rlen = ext4_rec_len_from_disk(de->rec_len,
> +							   blocksize);
> +
> +		if (nlen > rlen ||
> +		    rlen - nlen < ext4_dirent_rec_len(fname_len(fname), dir)) {
> +			EXT4_ERROR_INODE(dir,
> +				"no room for new entry in split block %llu",
> +				(unsigned long long)bh->b_blocknr);
> +			return -EFSCORRUPTED;
> +		}
>  	}

[Severity: High]
When this check in add_dirent_to_buf() returns -EFSCORRUPTED for a de
returned by do_split(), does it leave bh modified with a zeroed checksum
tail and without calling ext4_handle_dirty_dirblock()?

Before returning de to make_indexed_dir() or ext4_dx_add_entry(),
do_split() has already obtained write access to *bh, bh2, and frame->bh,
modified *bh and bh2 in-place via dx_move_dirents() and dx_pack_dirents(),
zeroed det_checksum in both blocks via ext4_initialize_dirent_tail(), and
dirtied bh2 and frame->bh:

fs/ext4/namei.c:do_split() {
    ...
	if (csum_size) {
		ext4_initialize_dirent_tail(*bh, blocksize);
		ext4_initialize_dirent_tail(bh2, blocksize);
	}
    ...
	if (hinfo->hash >= hash2) {
		swap(*bh, bh2);
		de = de2;
	}
	dx_insert_block(frame, hash2 + continued, newblock);
	err = ext4_handle_dirty_dirblock(handle, dir, bh2);
	if (err)
		goto journal_error;
	err = ext4_handle_dirty_dx_node(handle, dir, frame->bh);
	if (err)
		goto journal_error;
    ...
}

do_split() relies on add_dirent_to_buf() to checksum and dirty *bh via
ext4_handle_dirty_dirblock(handle, dir, bh). If add_dirent_to_buf() returns
-EFSCORRUPTED early here, bh2 and frame->bh are committed in the journal
with the post-split state (for instance in make_indexed_dir() or when
mounted with errors=continue), while bh is never passed to
ext4_handle_dirty_dirblock().

[1] https://lore.kernel.org/r/20261029122413.230409-8-ablagodarenko@thelustrecollective.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134921.610047-1-ablagodarenko@thelustrecollective.com?part=8