Re: [PATCH v3] 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:
- [High] ext4: do_split() index aliasing check misses ancestor htree frames
--

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

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

This commit adds checks in do_split() to verify that the directory leaf block
being split (*bh) and the newly allocated block (bh2) do not alias each other
or the htree index block (frame->bh) before moving directory entries.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..0789c9dc31b80 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1922,12 +1922,36 @@ 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);
>  
> +	if (*bh == frame->bh) {
> +		ext4_error_inode_block(dir, (*bh)->b_blocknr, EFSCORRUPTED,
> +				       "leaf block aliased with index block");
> +		brelse(*bh);
> +		*bh = NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}
> +
>  	bh2 = ext4_append(handle, dir, &newblock);
>  	if (IS_ERR(bh2)) {
>  		brelse(*bh);
>  		*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);
> +	}
> +	if (bh2 == frame->bh) {
> +		ext4_error_inode_block(dir, bh2->b_blocknr, EFSCORRUPTED,
> +				       "leaf block aliased with index block");
> +		brelse(bh2);
> +		brelse(*bh);
> +		*bh = NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}

[Severity: High]
As reported by Sashiko on v2 [1], what happens in a multi-level htree
directory (indirect_levels >= 1) when *bh or bh2 aliases an ancestor index
block such as frames[0].bh?

In ext4_dx_add_entry(), dx_probe() populates frames[0..indirect] and returns
frame = &frames[indirect], which is then passed to do_split():

fs/ext4/namei.c:ext4_dx_add_entry() {
	frame = dx_probe(fname, dir, NULL, frames);
	...
	de = do_split(handle, dir, &bh, frame, &fname->hinfo);
}

When indirect >= 1, frame->bh is the lowest-level interior index node
(frames[1].bh or frames[2].bh), while frames[0].bh holds the root index block
(struct dx_root, containing info.indirect_levels at byte offset 30). Because
do_split() only compares *bh and bh2 against frame->bh, an alias with
frames[0].bh (or frames[1].bh in a 3-level tree) is not caught.

Note that in a 1-level htree (indirect == 0, the only case where
frame == &frames[0]), frames[1].bh is NULL, so dx_release() breaks at i = 1
and cannot read past the stack array frames[EXT4_HTREE_LEVEL]. The
out-of-bounds read in dx_release() can only happen when all three frames are
populated (indirect == 2, where frame == &frames[2]):

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;
	}
}

If *bh or bh2 aliases frames[0].bh when indirect >= 1, dx_move_dirents() and
dx_pack_dirents() overwrite frames[0].bh->b_data, corrupting
info->indirect_levels. When ext4_dx_add_entry() then calls dx_release(frames),
could a smaller overwritten indirect_levels value cause dx_release() to stop
early and leak frames[1..indirect].bh, and could an overwritten value >= 3 in
a 3-level tree cause dx_release() to read past frames[] on the stack and pass
stack garbage to brelse()?

Also, should the commit message body be updated to mention the index block
aliasing checks added in v3?

[1] https://lore.kernel.org/r/tencent_7AA5D3581EE4D99A947097C4F02730DCCF09@qq.com

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