Re: [PATCH v5] ext4: fix slab-use-after-free in do_split()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] ext4: late alias check in make_indexed_dir() after buffer modification
- [Critical] ext4: unchecked buffer aliasing in ext4_dx_add_entry() index split
- [Critical] ext4: late leaf-to-index alias check in ext4_dx_add_entry()
--

commit eea9bdf87e428c4c24ab845db976b1362da00cdf
Author: Shuai Liu <847807867@qq.com>

ext4: fix slab-use-after-free in do_split()

This patch adds checks in do_split() to verify that neither the leaf directory
block being split (*bh) nor the newly allocated block (bh2) aliases each other
or any htree index block in frames[0..frame]. If an alias is detected,
do_split() logs an error and returns -EFSCORRUPTED before modifying directory
entries.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..878ed25ca4a2b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1903,13 +1903,14 @@ static struct ext4_dir_entry_2 *dx_pack_dirents(struct inode *dir, char *base,
>   * Returns pointer to de in block into which the new entry will be inserted.
>   */
>  static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
> -			struct buffer_head **bh,struct dx_frame *frame,
> -			struct dx_hash_info *hinfo)
> +			struct buffer_head **bh, struct dx_frame *frames,
> +			struct dx_frame *frame, struct dx_hash_info *hinfo)
>  {
>  	unsigned blocksize = dir->i_sb->s_blocksize;
>  	unsigned continued;
>  	int count;
>  	struct buffer_head *bh2;
> +	struct dx_frame *f;
>  	ext4_lblk_t newblock;
>  	u32 hash2;
>  	struct dx_map_entry *map;
> @@ -1922,12 +1923,42 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>  	if (ext4_has_feature_metadata_csum(dir->i_sb))
>  		csum_size = sizeof(struct ext4_dir_entry_tail);
>  
> +	for (f = frames; f <= frame; f++) {
> +		if (*bh == f->bh) {
> +			ext4_error_inode_block(dir, (*bh)->b_blocknr,
> +					       EFSCORRUPTED,
> +					       "leaf block aliased with index block");
> +			brelse(*bh);
> +			*bh = NULL;
> +			return ERR_PTR(-EFSCORRUPTED);
> +		}
> +	}

[Severity: Critical]
Is checking for *bh == f->bh inside do_split() too late when called from
ext4_dx_add_entry()?

In ext4_dx_add_entry(), after reading the leaf block bh, add_dirent_to_buf()
is called before do_split() is ever reached:

    bh = ext4_read_dirblock(dir, dx_get_block(frame->at), DIRENT_HTREE);
    if (IS_ERR(bh)) {
        err = PTR_ERR(bh);
        bh = NULL;
        goto cleanup;
    }
    ...
    err = add_dirent_to_buf(handle, fname, dir, inode, NULL, bh);
    if (err != -ENOSPC)
        goto cleanup;

If bh aliases one of the index buffers in frames[0..frame].bh, dx_probe() has
already marked that buffer verified, so __ext4_read_dirblock() returns it
without running leaf checksum verification. Because an htree root block has
root->dotdot->rec_len == blocksize - 12 (and an internal dx_node has
node->fake.rec_len == blocksize), on a filesystem without metadata_csum,
ext4_find_dest_de() finds space inside that fake dirent and returns 0 instead
of -ENOSPC.

ext4_insert_dentry() then writes the new directory entry directly over the
htree header (for example, overwriting root->info.indirect_levels at offset 30
of frames[0].bh->b_data with de1->name_len), and ext4_dx_add_entry() jumps to
cleanup without calling do_split(). In dx_release(), the loop:

    indirect_levels = info->indirect_levels;
    for (i = 0; i <= indirect_levels; i++) {
        if (frames[i].bh == NULL)
            break;
        brelse(frames[i].bh);
        frames[i].bh = NULL;
    }

can then read past the end of the frames[EXT4_HTREE_LEVEL] stack array when
all frames are populated, or leak frame buffers if indirect_levels was
overwritten with a smaller value. Should the leaf-to-index alias check happen
in ext4_dx_add_entry() right after ext4_read_dirblock()?

[ ... ]

> @@ -2325,7 +2356,7 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>  	if (retval)
>  		goto out_frames;
>  
> -	de = do_split(handle,dir, &bh2, frame, &fname->hinfo);
> +	de = do_split(handle, dir, &bh2, frames, frame, &fname->hinfo);
>  	if (IS_ERR(de)) {
>  		retval = PTR_ERR(de);
>  		goto out_frames;

[Severity: Critical]
In make_indexed_dir(), frame->bh is bh (logical block 0) and the buffer passed
to do_split() is bh2, which was allocated earlier in make_indexed_dir() via
ext4_append():

    bh2 = ext4_append(handle, dir, &block);
    if (IS_ERR(bh2)) {
        brelse(bh);
        return PTR_ERR(bh2);
    }
    ext4_set_inode_flag(dir, EXT4_INODE_INDEX);
    data2 = bh2->b_data;

    memcpy(data2, de, len);
    memset(de, 0, len); /* wipe old data */

If block 0 is marked free in a corrupted block bitmap and ext4_append()
returns bh2 == bh, won't make_indexed_dir() perform an overlapping memcpy()
over root->dot and root->dotdot, wipe the copied entries with memset(),
reinitialize bh->b_data as a dx_root header, and dirty both buffers in the
journal before do_split() ever checks *bh == f->bh?

> @@ -2631,7 +2662,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			goto journal_error;
>  		}
>  	}
> -	de = do_split(handle, dir, &bh, frame, &fname->hinfo);
> +	de = do_split(handle, dir, &bh, frames, frame, &fname->hinfo);
>  	if (IS_ERR(de)) {
>  		err = PTR_ERR(de);
>  		goto cleanup;

[Severity: Critical]
What happens if the index split right above this do_split() call in
ext4_dx_add_entry() receives an aliased buffer from ext4_append()?

When dx_get_count(entries) == dx_get_limit(entries), ext4_dx_add_entry()
allocates a new index block:

    bh2 = ext4_append(handle, dir, &newblock);
    if (IS_ERR(bh2)) {
        err = PTR_ERR(bh2);
        goto cleanup;
    }
    node2 = (struct dx_node *)(bh2->b_data);
    entries2 = node2->entries;
    memset(&node2->fake, 0, sizeof(struct fake_dirent));

Nothing checks whether bh2 aliases frames[0..level].bh or the leaf block bh:

1. In the !add_level branch, if bh2 aliases (frame - 1)->bh, the memcpy() into
   entries2 overwrites (frame - 1)->entries, and the subsequent call to
   dx_insert_block((frame - 1), hash2, newblock) reads the corrupted count and
   can memmove() out of bounds past the end of (frame - 1)->bh->b_data.

2. In the add_level branch, if bh2 aliases frames[0].bh, the memcpy() from
   entries (offset 32) to entries2 (offset 8) is an overlapping copy within
   frames[0].bh->b_data that overwrites dxroot->info.indirect_levels at offset
   30, increments it, and jumps directly to journal_error / cleanup without
   reaching do_split(), causing dx_release(frames) to loop out of bounds past
   frames[EXT4_HTREE_LEVEL] or leak frame references.

3. In the !add_level branch when restart == 0 and at - entries < icount1, if
   bh2 aliases the leaf block bh, bh->b_data is overwritten with a dx_node
   header and index entries, and bh2 is released without swapping into
   frame->bh, so do_split() then runs on the corrupted leaf buffer without
   detecting the alias.

Could the alias checks be performed when bh2 is allocated during the index
split as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_4EAEF2024D7BB83D62E41306EED8D2112C08@qq.com?part=1