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

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

[Shuai Liu]

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]

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

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@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 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@suse.com>
SUSE Labs, CR

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

[Shuai Liu]

Hi Honza,

Thank you for the detailed review and the honest feedback.

You are right - I was iterating against the AI review bot's findings
without pausing to seek human feedback, and that was the wrong
prioritization. I also agree with your point on maintainability: disk
metadata can be corrupted in unbounded ways, ext4 has always treated
graceful handling as best effort, and adding per-alias checks at every
call site is expensive protection for a niche case.

Given that, how would you like to proceed? The syzbot crash itself is
real (KASAN slab-use-after-free from aliased buffer heads), so I could
downscale this to a minimal fix for the actual reported crash (just the
do_split() *bh/bh2 alias check) if you think that single check is
worthwhile. Alternatively I can drop the series entirely, and I would
be interested in helping explore the buffer_head metadata tag approach
you outlined, if that is something you would welcome.

Thanks again,
Shuai Liu