Re: [PATCH v4] 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() misses aliasing checks against ancestor htree index frames
--

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

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

This commit adds aliasing checks in do_split() to prevent splitting a
directory leaf block onto itself or onto an htree index block when the block
bitmap is inconsistent with the directory tree.

>     ext4: fix slab-use-after-free in do_split()
>     
>     do_split() allocates a new directory block with ext4_append() to hold
>     the second half of the entries of the leaf block being split, then
>     dx_move_dirents() copies entries from the old block (*bh) into the new
>     one (bh2).  Nothing verifies that the newly allocated block differs
>     from the block being split.
>     
>     On a filesystem whose block bitmap is inconsistent with the directory
>     tree, a block already referenced as a directory leaf or as an htree
>     index block can still be marked free, so ext4_append() can return that
>     same block.  With concurrent operations on such a filesystem (as
>     exercised by the syzbot reproducer), the block can even be freed
>     between allocations, resulting in bh2 aliasing *bh.  dx_move_dirents()
>     then copies the block onto itself, corrupting directory entries
>     mid-copy: a rec_len field can be overwritten with name bytes, so the
>     subsequent wipe of the stale entry memset()s a bogus length, causing
>     the slab-use-after-free reported by syzbot.
>     
>     The same aliasing applies to the htree index blocks: if the leaf block
>     or the new block aliases any index frame (the parent index block
>     frame->bh or one of its ancestors in the frames[] array),
>     dx_move_dirents() and dx_pack_dirents() overwrite the index header,
>     dx_insert_block() then memmove()s with a corrupted count and writes
>     out of bounds, and dx_release() can loop past the frames[] array (or
>     leak frames) with a corrupted indirect_levels.
>     
>     Fix this by checking for all of these aliases in do_split() and
>     bailing out with -EFSCORRUPTED before any directory block contents are
>     modified, reporting the corruption via ext4_error_inode_block().
>     
>     Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com
>     Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
>     Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Shuai Liu <847807867@qq.com>
>     Changes in v4:
>       - Extend the aliasing checks to all htree index frames: do_split()
>         now takes the frames[] array and compares *bh and bh2 against
>         frames[0..level], not just the parent frame.  An alias with an
>         ancestor index block would overwrite its header, letting
>         dx_insert_block() memmove() out of bounds and dx_release() loop
>         past the frames[] array or leak frames (Sashiko)
>       - Mention the index block aliasing checks in the commit message
>         (Sashiko)
> 
> 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 v3 [1], what happens in a multi-level htree
directory (when root->info.indirect_levels >= 1) if *bh or bh2 aliases an
ancestor index block in frames[0..indirect-1].bh rather than the deepest
index block in frame->bh?

Although the commit message and the embedded "Changes in v4" changelog state
that do_split() now takes the frames[] array and compares *bh and bh2 against
frames[0..level], the code in fs/ext4/namei.c was not updated to match:

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

Because ext4_dx_add_entry() still passes only frame (&frames[indirect]) and
do_split() only compares *bh == frame->bh and bh2 == frame->bh, any alias
between *bh or bh2 and frames[0].bh (or frames[1].bh when indirect == 2) goes
undetected.

When frames[0].bh is overwritten with directory entries by dx_move_dirents()
and dx_pack_dirents(), ((struct dx_root *)frames[0].bh->b_data)->info is
corrupted before ext4_dx_add_entry() calls dx_release(frames):

fs/ext4/namei.c:dx_release() {
	if (frames[0].bh == NULL)
		return;

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

Can this corrupt info->indirect_levels so that dx_release() either stops
early and leaks the buffer_head references in frames[1..indirect].bh, or (when
all three entries in frames[EXT4_HTREE_LEVEL] are populated) reads past the
end of the stack-allocated frames[] array and passes stack garbage to
brelse()?

Also, should the "Changes in v4:" block be moved below the "---" separator so
it is not included in the commit message body?

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

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