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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ext4: buffer_head reference leak in do_split() when bh2 aliases *bh
- [Medium] ext4: negative errno and logical block passed to ext4_error_inode_block()
--

commit 03492d237958226b95dd674bad3a9c3663bcb8b7
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_head (bh2) does not alias the block being split (*bh),
returning -EFSCORRUPTED if they match.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..bffd4c460842a 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1928,6 +1928,13 @@ 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, newblock, -EFSCORRUPTED,
> +				       "cannot split aliased directory block");

[Severity: Medium]
Should ext4_error_inode_block() take (*bh)->b_blocknr and a positive errno
(or 0, which save_error_info() normalizes to EFSCORRUPTED) instead of
newblock and -EFSCORRUPTED?

ext4_error_inode_block() passes error and block to __ext4_error_inode() ->
ext4_handle_error():

fs/ext4/super.c:ext4_handle_error() {
	...
	if (!continue_fs && !ext4_emergency_ro(sb) && journal)
		jbd2_journal_abort(journal, -error);

	if (!bdev_read_only(sb->s_bdev)) {
		save_error_info(sb, error, ino, block, func, line);
	...
}

When error is negative (-EFSCORRUPTED), jbd2_journal_abort(journal, -error)
receives a positive value instead of a negative errno, and
save_error_info() -> __save_error_info() -> ext4_errno_to_code() fails to
match -EFSCORRUPTED against the positive errno entries in err_translation[],
recording EXT4_ERR_UNKNOWN in the superblock.

In addition, the block parameter of ext4_error_inode_block() expects a
physical block number (ext4_fsblk_t) such as (*bh)->b_blocknr rather than the
logical directory block offset newblock, matching the existing call later in
do_split():

	if (WARN_ON_ONCE(split == 0)) {
		/* Should never happen, but avoid out-of-bounds access below */
		ext4_error_inode_block(dir, (*bh)->b_blocknr, 0,
			"bad indexed directory? hash=%08x:%08x count=%d move=%u",
			hinfo->hash, hinfo->minor_hash, count, move);
		err = -EFSCORRUPTED;
		goto out;
	}

> +		brelse(bh2);
> +		*bh = NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}

[Severity: Medium]
Does this path leak a buffer_head reference on *bh?

When do_split() is entered from ext4_dx_add_entry() or make_indexed_dir(),
the caller holds a reference on *bh. Inside do_split(), ext4_append() ->
ext4_bread() -> ext4_getblk() -> getblk_unmovable() looks up the buffer_head
for the newly allocated block and increments b_count. When bh2 == *bh, the
same buffer_head now has two references held on it: one from *bh and one from
bh2.

Calling brelse(bh2) only drops one reference, and setting *bh = NULL prevents
the caller's error cleanup (such as brelse(bh) at cleanup: in
ext4_dx_add_entry()) from dropping the original reference.

Could this jump to out: (or call brelse(*bh) as well) so that both references
are released before *bh is cleared:

out:
	brelse(*bh);
	brelse(bh2);
	*bh = NULL;
	return ERR_PTR(err);

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