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

[Jan Kara]

On Thu 05-11-26 10:11:46, Shuai Liu wrote:
> 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 a newly allocated block aliases an index frame, the index
> header can be overwritten before do_split() runs (add_dirent_to_buf()
> inserting into an aliased leaf, make_indexed_dir() moving the dirents
> into an aliased new block, the index split in ext4_dx_add_entry()
> copying entries into an aliased buffer), letting dx_insert_block()
> memmove() out of bounds with a corrupted count and dx_release() loop
> past the frames[] array (or leak frames) with a corrupted
> indirect_levels.
> 
> Fix this by checking for these aliases before any directory block
> contents are modified, and bailing out with -EFSCORRUPTED, reporting
> the corruption via ext4_error_inode_block():
> 
>   - make_indexed_dir(): after ext4_append(), before moving the
>     dirents out of the root block
>   - ext4_dx_add_entry(): after reading the leaf block and after
>     ext4_append() in the index split, before the entries are copied
>   - do_split(): for *bh and bh2 against all frames[0..level]

So you are at v8 in just two days without any human feedback. This isn't a
good investment of your time. Did you put actually more thought into
whether what AI is suggesting to do actually makes sense? Because I don't
think it does. There is a very large number of ways in which metadata on disk
can be corrupted and handling them gracefully was always declared as best
effort because some types of corruption are simply too expensive to protect
against. This is fixing one niche case with relatively complex code so
that's not really a win for long term code maintainability.

As I've outlined elsewhere this class of corruptions could likely be
addressed by attaching a tag to buffer head after verifying its contents
saying what metadata it holds and verifying the tag is the expected one
whenever we lookup metadata bh from buffer cache.

								Honza

> Reported-by: syzbot+09bec78ee77613a3efdd@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Shuai Liu <847807867@xxxxxx>
> Changes in v8:
>   - Add an index-to-index alias check in dx_probe(). The existing tree cycle
>     check compares only logical block numbers, so on a corrupted filesystem
>     where two distinct logical index blocks map to the same physical block,
>     frames[] can be populated with identical buffer_heads (the same physical
>     block can pass the limit, count and checksum checks at both levels since
>     dx_root and dx_node entries live at different offsets). Writing a child
>     frame's dx_node entries in place then overwrites the root dx_root_info
>     (including indirect_levels) and the root dx_countlimit, and dx_release()
>     reading the corrupted indirect_levels can walk past the end of the
>     frames[] array and brelse() stack garbage. Check each newly read index
>     block against all ancestor frames' buffer_heads, which also removes a
>     double brelse() of an aliased buffer_head on the dx_probe() failure
>     path. (Sashiko)
> 
> ---
>  fs/ext4/namei.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 354fc2d22df1..fb1ea835cd20 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -914,6 +914,17 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
>  			goto fail;
>  		}
>  
> +		for (i = 0; i < level; i++) {
> +			if (frame_in[i].bh == frame->bh) {
> +				ext4_warning_inode(dir,
> +					"dx entry: index block %u aliases ancestor index block",
> +					block);
> +				brelse(frame->bh);
> +				frame->bh = NULL;
> +				goto fail;
> +			}
> +		}
> +
>  		entries = ((struct dx_node *) frame->bh->b_data)->entries;
>  
>  		if (dx_get_limit(entries) != dx_node_limit(dir)) {
> @@ -1903,13 +1914,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 +1934,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);
> +		}
> +	}
> +
>  	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);
> +	}
> +	for (f = frames; f <= frame; f++) {
> +		if (bh2 == f->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);
> +		}
> +	}
>  
>  	BUFFER_TRACE(*bh, "get_write_access");
>  	err = ext4_journal_get_write_access(handle, dir->i_sb, *bh,
> @@ -2257,6 +2299,13 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>  		brelse(bh);
>  		return PTR_ERR(bh2);
>  	}
> +	if (bh2 == bh) {
> +		ext4_error_inode_block(dir, bh->b_blocknr, EFSCORRUPTED,
> +				       "cannot index aliased directory block");
> +		brelse(bh2);
> +		brelse(bh);
> +		return -EFSCORRUPTED;
> +	}
>  	ext4_set_inode_flag(dir, EXT4_INODE_INDEX);
>  	data2 = bh2->b_data;
>  
> @@ -2325,7 +2374,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;
> @@ -2476,6 +2525,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			     struct inode *dir, struct inode *inode)
>  {
>  	struct dx_frame frames[EXT4_HTREE_LEVEL], *frame;
> +	struct dx_frame *f;
>  	struct dx_entry *entries, *at;
>  	struct buffer_head *bh;
>  	struct super_block *sb = dir->i_sb;
> @@ -2496,6 +2546,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  		bh = NULL;
>  		goto cleanup;
>  	}
> +	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");
> +			err = -EFSCORRUPTED;
> +			goto cleanup;
> +		}
> +	}
>  
>  	BUFFER_TRACE(bh, "get_write_access");
>  	err = ext4_journal_get_write_access(handle, sb, bh, EXT4_JTR_NONE);
> @@ -2549,6 +2608,18 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			err = PTR_ERR(bh2);
>  			goto cleanup;
>  		}
> +		for (f = frames; f < frames + levels; f++) {
> +			if (bh2 == f->bh)
> +				break;
> +		}
> +		if (f < frames + levels || bh2 == bh) {
> +			ext4_error_inode_block(dir, bh2->b_blocknr,
> +					       EFSCORRUPTED,
> +					       "cannot split aliased index block");
> +			brelse(bh2);
> +			err = -EFSCORRUPTED;
> +			goto cleanup;
> +		}
>  		node2 = (struct dx_node *)(bh2->b_data);
>  		entries2 = node2->entries;
>  		memset(&node2->fake, 0, sizeof(struct fake_dirent));
> @@ -2631,7 +2702,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;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR