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

[PATCH v6] 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 v6:
  - Move the aliasing checks to every point where a directory buffer
    is obtained for an htree operation, before its contents are
    modified, as the checks inside do_split() were too late for three
    paths (Sashiko):
    * make_indexed_dir(): an aliased ext4_append() result would be
      memcpy()ed and memset()ed before do_split() could check it
    * ext4_dx_add_entry(): add_dirent_to_buf() would insert into a
      leaf block aliasing an already-verified index block right after
      the read, and the index split would copy entries into an
      aliased ext4_append() buffer before do_split() is reached
  - Extend the commit message accordingly (Sashiko)

---
 fs/ext4/namei.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..d9ae5c5a5562 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1903,13 +1903,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 +1923,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 +2288,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 +2363,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 +2514,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 +2535,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 +2597,18 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 			err = PTR_ERR(bh2);
 			goto cleanup;
 		}
+		for (f = frames; f <= frame; f++) {
+			if (bh2 == f->bh)
+				break;
+		}
+		if (f <= frame || 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 +2691,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 v6] ext4: fix slab-use-after-free in do_split()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: incomplete bh2 alias check in ext4_dx_add_entry() index split
--

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

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

This commit adds buffer_head aliasing checks in make_indexed_dir(),
ext4_dx_add_entry(), and do_split() to prevent directory and htree index
blocks from aliasing each other when allocating or reading blocks on a
filesystem with a corrupted block bitmap.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..79f7f0073d9da 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c

[ ... ]

> @@ -2549,6 +2597,18 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			err = PTR_ERR(bh2);
>  			goto cleanup;
>  		}
> +		for (f = frames; f <= frame; f++) {
> +			if (bh2 == f->bh)
> +				break;
> +		}
> +		if (f <= frame || bh2 == bh) {
> +			ext4_error_inode_block(dir, bh2->b_blocknr,
> +					       EFSCORRUPTED,
> +					       "cannot split aliased index block");
> +			brelse(bh2);
> +			err = -EFSCORRUPTED;
> +			goto cleanup;
> +		}

[Severity: High]
As reported by Sashiko on v5 [1], in ext4_dx_add_entry() in fs/ext4/namei.c,
when the leaf index block is full and an ancestor index block is also full,
the preceding loop walks up the htree and decrements frame:

	if (dx_get_count(entries) == dx_get_limit(entries)) {
		ext4_lblk_t newblock;
		int levels = frame - frames + 1;
		...
		while (frame > frames) {
			if (dx_get_count((frame - 1)->entries) <
			    dx_get_limit((frame - 1)->entries)) {
				add_level = 0;
				break;
			}
			frame--; /* split higher index block */
			at = frame->at;
			entries = frame->entries;
			restart = 1;
		}

By the time ext4_append() allocates bh2, frame may point to frames[0] (or an
intermediate frame) while the lower-level index blocks in
frames[(frame - frames) + 1 .. levels - 1] remain active in the htree and held
in frames[] until dx_release(frames) runs at cleanup.

Because the alias check loop bounds f by frame rather than
frames + levels - 1 (and testing f < frames + levels), can bh2 alias one of
those skipped lower-level index frames (such as frames[1].bh when splitting
the root index)? If so, wouldn't the subsequent memset and memcpy into
node2 overwrite the active lower index buffer in place and create a cycle in
the htree before jumping to again?

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

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

[PATCH v7] 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 v7:
  - Extend the index split alias check to all active htree frames
    (frames[0..levels-1]), not just the current frame and its
    ancestors: after the parent index walkback loop decrements frame,
    the skipped lower-level frames stay active and are still released
    by dx_release(), so an aliased bh2 could overwrite them in place
    (Sashiko)

---
 fs/ext4/namei.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..55892206dbd8 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1903,13 +1903,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 +1923,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 +2288,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 +2363,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 +2514,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 +2535,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 +2597,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 +2691,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 v7] ext4: fix slab-use-after-free in do_split()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: missing index-to-index alias check in dx_probe() and ext4_dx_add_entry()
--

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

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

This commit adds buffer_head alias checks in do_split(), make_indexed_dir(),
and ext4_dx_add_entry() to prevent directory leaf blocks and newly allocated
blocks from aliasing each other or active htree index frames on corrupted
filesystems.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..fadc2de1beb1c 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c

[ ... ]

> @@ -2496,6 +2535,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 +2597,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;
> +		}

[Severity: High]
What happens if two active htree index frames in frames[] already alias the
same physical block when dx_probe() returns?

In dx_probe(), the cycle check only compares logical block numbers in
blocks[]:

	block = dx_get_block(at);
	for (i = 0; i <= level; i++) {
		if (blocks[i] == block) {
			ext4_warning_inode(dir,
				"dx entry: tree cycle block %u points back to block %u",
				blocks[level], block);
			goto fail;
		}
	}
	if (++level > indirect)
		return frame;
	blocks[level] = block;
	frame++;
	frame->bh = ext4_read_dirblock(dir, block, INDEX);

On a corrupted filesystem where two distinct logical index blocks (such as
logical block 0 for frames[0] and logical block 1 for frames[1]) map to the
same physical disk block, dx_probe() can populate frames[] with identical
buffer_head pointers (frames[0].bh == frames[1].bh) becauseframes[0].entries
is read at offset 32 (after dx_root_info) while the child dx_node entries
array is read at offset 8, allowing a single physical block to pass the
limit, count, and checksum checks for both levels.

The new checks in ext4_dx_add_entry() and do_split() verify that the leaf
block bh and newly appended block bh2 do not alias f->bh, but nothing checks
whether the active index frames in frames[0..frame] alias each other.

When do_split() or the interior index split in ext4_dx_add_entry() calls
dx_insert_block() on a child frame whose bh aliases frames[0].bh, shifting or
writing dx_node entries starting at offset 8 overwrites frames[0]'s
dx_root_info (including info.indirect_levels) and root dx_countlimit in
place. When dx_release() subsequently runs:

	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 and cause dx_release() to either leak
lower-level frame buffer_heads or read past the end of frames[EXT4_HTREE_LEVEL]
when all frames are active? Would checking that frame->bh does not alias any
ancestor frames[0..level-1].bh in dx_probe() prevent index-to-index aliasing
as well?

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

[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()

[sashiko-bot]

> 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
> [ ... ]
> 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>

Sashiko has reviewed this patch and found no issues.

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

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