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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] ext4: missing frame->bh aliasing check in do_split() causes OOB write
--

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

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

This commit adds a check in do_split() to verify that the newly allocated
directory block buffer bh2 returned by ext4_append() does not alias the leaf
block *bh being split, returning -EFSCORRUPTED if they match.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..c0454a2594523 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1928,6 +1928,14 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>  		*bh = NULL;
>  		return ERR_CAST(bh2);
>  	}
> +	if (bh2 == *bh) {
> +		ext4_error_inode_block(dir, (*bh)->b_blocknr, EFSCORRUPTED,
> +				       "cannot split aliased directory block");
> +		brelse(bh2);
> +		brelse(*bh);
> +		*bh = NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}

[Severity: Critical]
What happens if bh2 or *bh aliases frame->bh under the same inconsistent
block bitmap or block reuse condition?

In do_split(), frame->bh holds the htree index block and frame->entries
points into frame->bh->b_data. If ext4_append() returns bh2 == frame->bh
while bh2 != *bh, this check passes and data2 is set to bh2->b_data:

fs/ext4/namei.c:do_split() {
    ...
    data2 = bh2->b_data;
    ...
    de2 = dx_move_dirents(dir, data1, data2, map + split, count - split,
                          blocksize);
    ...
    dx_insert_block(frame, hash2 + continued, newblock);
}

dx_move_dirents() then copies directory entries into frame->bh->b_data,
overwriting the htree index header and frame->entries (or dx_pack_dirents()
overwrites it in data1 if *bh == frame->bh).

When dx_insert_block() subsequently reads the corrupted count and limit from
frame->entries:

fs/ext4/namei.c:dx_insert_block() {
    ...
    int count = dx_get_count(entries);

    ASSERT(count < dx_get_limit(entries));
    ASSERT(old < entries + count);
    memmove(new + 1, new, (char *)(entries + count) - (char *)(new));
    ...
}

it can trigger BUG() via ASSERT() or perform an out-of-bounds memmove() past
bh2->b_data.

Furthermore, if frame is &frames[0], overwriting frames[0].bh->b_data
corrupts ((struct dx_root *)frames[0].bh->b_data)->info.indirect_levels with
directory entry bytes, causing dx_release() to read out of bounds past
frames[EXT4_HTREE_LEVEL] on the stack and call brelse() on arbitrary stack
values:

fs/ext4/namei.c:dx_release() {
    ...
    info = &((struct dx_root *)frames[0].bh->b_data)->info;
    /* save local copy, "info" may be freed after brelse() */
    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;
    }
}

Should do_split() also check whether bh2 == frame->bh or *bh == frame->bh?

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