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

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

Fix this by checking for bh2 == *bh right after ext4_append() 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>
---
 fs/ext4/namei.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..4ef7983d46c6 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");
+		brelse(bh2);
+		*bh = NULL;
+		return ERR_PTR(-EFSCORRUPTED);
+	}
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, dir->i_sb, *bh,
-- 
2.43.0

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

[sashiko-bot]

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

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

Fix this by checking for bh2 == *bh right after ext4_append() 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 v2:
  - Release both buffer_head references when bh2 aliases *bh to avoid
    leaking one reference (Sashiko)
  - Pass the physical block number instead of the logical block number
    to ext4_error_inode_block() (Sashiko)
  - Pass a positive errno to ext4_error_inode_block(), as is the
    convention (Sashiko)

 fs/ext4/namei.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..c664ae84320a 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1928,6 +1928,14 @@ 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, (*bh)->b_blocknr, EFSCORRUPTED,
+				       "cannot split aliased directory 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,
-- 
2.43.0

Re: [PATCH v2] 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:
- [Critical] ext4: missing frame->bh aliasing check in do_split() causes OOB write
--

commit 030c053bcea32f45b1d61f119ec600fe4bd02c71
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 bh2 returned by ext4_append() does not alias the leaf
block *bh being split, returning -EFSCORRUPTED if they match.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..c0454a2594523 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1928,6 +1928,14 @@ 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, (*bh)->b_blocknr, EFSCORRUPTED,
> +				       "cannot split aliased directory block");
> +		brelse(bh2);
> +		brelse(*bh);
> +		*bh = NULL;
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}

[Severity: Critical]
What happens if bh2 or *bh aliases frame->bh under the same inconsistent
block bitmap or block reuse condition?

In do_split(), frame->bh holds the htree index block and frame->entries
points into frame->bh->b_data. If ext4_append() returns bh2 == frame->bh
while bh2 != *bh, this check passes and data2 is set to bh2->b_data:

fs/ext4/namei.c:do_split() {
    ...
    data2 = bh2->b_data;
    ...
    de2 = dx_move_dirents(dir, data1, data2, map + split, count - split,
                          blocksize);
    ...
    dx_insert_block(frame, hash2 + continued, newblock);
}

dx_move_dirents() then copies directory entries into frame->bh->b_data,
overwriting the htree index header and frame->entries (or dx_pack_dirents()
overwrites it in data1 if *bh == frame->bh).

When dx_insert_block() subsequently reads the corrupted count and limit from
frame->entries:

fs/ext4/namei.c:dx_insert_block() {
    ...
    int count = dx_get_count(entries);

    ASSERT(count < dx_get_limit(entries));
    ASSERT(old < entries + count);
    memmove(new + 1, new, (char *)(entries + count) - (char *)(new));
    ...
}

it can trigger BUG() via ASSERT() or perform an out-of-bounds memmove() past
bh2->b_data.

Furthermore, if frame is &frames[0], overwriting frames[0].bh->b_data
corrupts ((struct dx_root *)frames[0].bh->b_data)->info.indirect_levels with
directory entry bytes, causing dx_release() to read out of bounds past
frames[EXT4_HTREE_LEVEL] on the stack and call brelse() on arbitrary stack
values:

fs/ext4/namei.c:dx_release() {
    ...
    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;
    }
}

Should do_split() also check whether bh2 == frame->bh or *bh == frame->bh?

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

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

Fix this by checking for bh2 == *bh right after ext4_append() 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 v3:
  - Also check for aliasing with the htree index block, as requested
    by Sashiko: check *bh == frame->bh before the allocation and
    bh2 == frame->bh after it.  An aliased index block would be
    overwritten by dx_move_dirents() / dx_pack_dirents(), letting
    dx_insert_block() memmove() out of bounds with a corrupted count
    and dx_release() brelse() garbage frames read past the stack
    array.

 fs/ext4/namei.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..25bea69ab246 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);
+	}
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, dir->i_sb, *bh,
-- 
2.43.0

Re: [PATCH v3] 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: do_split() index aliasing check misses ancestor htree frames
--

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

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

This commit adds checks in do_split() to verify that the directory leaf block
being split (*bh) and the newly allocated block (bh2) do not alias each other
or the htree index block (frame->bh) before moving directory entries.

> 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 v2 [1], what happens in a multi-level htree
directory (indirect_levels >= 1) when *bh or bh2 aliases an ancestor index
block such as frames[0].bh?

In ext4_dx_add_entry(), dx_probe() populates frames[0..indirect] and returns
frame = &frames[indirect], which is then passed to do_split():

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

When indirect >= 1, frame->bh is the lowest-level interior index node
(frames[1].bh or frames[2].bh), while frames[0].bh holds the root index block
(struct dx_root, containing info.indirect_levels at byte offset 30). Because
do_split() only compares *bh and bh2 against frame->bh, an alias with
frames[0].bh (or frames[1].bh in a 3-level tree) is not caught.

Note that in a 1-level htree (indirect == 0, the only case where
frame == &frames[0]), frames[1].bh is NULL, so dx_release() breaks at i = 1
and cannot read past the stack array frames[EXT4_HTREE_LEVEL]. The
out-of-bounds read in dx_release() can only happen when all three frames are
populated (indirect == 2, where frame == &frames[2]):

fs/ext4/namei.c:dx_release() {
	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;
	}
}

If *bh or bh2 aliases frames[0].bh when indirect >= 1, dx_move_dirents() and
dx_pack_dirents() overwrite frames[0].bh->b_data, corrupting
info->indirect_levels. When ext4_dx_add_entry() then calls dx_release(frames),
could a smaller overwritten indirect_levels value cause dx_release() to stop
early and leak frames[1..indirect].bh, and could an overwritten value >= 3 in
a 3-level tree cause dx_release() to read past frames[] on the stack and pass
stack garbage to brelse()?

Also, should the commit message body be updated to mention the index block
aliasing checks added in v3?

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

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

[PATCH v4] 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 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)
---
 fs/ext4/namei.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..25bea69ab246 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);
+	}
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, dir->i_sb, *bh,
-- 
2.43.0

Re: [PATCH v4] 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: 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

[PATCH v5] 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 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 v5:
  - The v4 posting mistakenly carried the v3 diff (the code changes
    were left unstaged when the commit was amended), so the code did
    not match the v4 changelog.  The code now actually implements
    those changes: do_split() takes the frames[] array and checks *bh
    and bh2 against all frames[0..level] (spotted by Sashiko)

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

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 354fc2d22df1..5f2790a0b8b4 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,
@@ -2325,7 +2356,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;
@@ -2631,7 +2662,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 v5] ext4: fix slab-use-after-free in do_split()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] ext4: late alias check in make_indexed_dir() after buffer modification
- [Critical] ext4: unchecked buffer aliasing in ext4_dx_add_entry() index split
- [Critical] ext4: late leaf-to-index alias check in ext4_dx_add_entry()
--

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

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

This patch adds checks in do_split() to verify that neither the leaf directory
block being split (*bh) nor the newly allocated block (bh2) aliases each other
or any htree index block in frames[0..frame]. If an alias is detected,
do_split() logs an error and returns -EFSCORRUPTED before modifying directory
entries.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 231a274b814ec..878ed25ca4a2b 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);
> +		}
> +	}

[Severity: Critical]
Is checking for *bh == f->bh inside do_split() too late when called from
ext4_dx_add_entry()?

In ext4_dx_add_entry(), after reading the leaf block bh, add_dirent_to_buf()
is called before do_split() is ever reached:

    bh = ext4_read_dirblock(dir, dx_get_block(frame->at), DIRENT_HTREE);
    if (IS_ERR(bh)) {
        err = PTR_ERR(bh);
        bh = NULL;
        goto cleanup;
    }
    ...
    err = add_dirent_to_buf(handle, fname, dir, inode, NULL, bh);
    if (err != -ENOSPC)
        goto cleanup;

If bh aliases one of the index buffers in frames[0..frame].bh, dx_probe() has
already marked that buffer verified, so __ext4_read_dirblock() returns it
without running leaf checksum verification. Because an htree root block has
root->dotdot->rec_len == blocksize - 12 (and an internal dx_node has
node->fake.rec_len == blocksize), on a filesystem without metadata_csum,
ext4_find_dest_de() finds space inside that fake dirent and returns 0 instead
of -ENOSPC.

ext4_insert_dentry() then writes the new directory entry directly over the
htree header (for example, overwriting root->info.indirect_levels at offset 30
of frames[0].bh->b_data with de1->name_len), and ext4_dx_add_entry() jumps to
cleanup without calling do_split(). In dx_release(), the loop:

    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 then read past the end of the frames[EXT4_HTREE_LEVEL] stack array when
all frames are populated, or leak frame buffers if indirect_levels was
overwritten with a smaller value. Should the leaf-to-index alias check happen
in ext4_dx_add_entry() right after ext4_read_dirblock()?

[ ... ]

> @@ -2325,7 +2356,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;

[Severity: Critical]
In make_indexed_dir(), frame->bh is bh (logical block 0) and the buffer passed
to do_split() is bh2, which was allocated earlier in make_indexed_dir() via
ext4_append():

    bh2 = ext4_append(handle, dir, &block);
    if (IS_ERR(bh2)) {
        brelse(bh);
        return PTR_ERR(bh2);
    }
    ext4_set_inode_flag(dir, EXT4_INODE_INDEX);
    data2 = bh2->b_data;

    memcpy(data2, de, len);
    memset(de, 0, len); /* wipe old data */

If block 0 is marked free in a corrupted block bitmap and ext4_append()
returns bh2 == bh, won't make_indexed_dir() perform an overlapping memcpy()
over root->dot and root->dotdot, wipe the copied entries with memset(),
reinitialize bh->b_data as a dx_root header, and dirty both buffers in the
journal before do_split() ever checks *bh == f->bh?

> @@ -2631,7 +2662,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;

[Severity: Critical]
What happens if the index split right above this do_split() call in
ext4_dx_add_entry() receives an aliased buffer from ext4_append()?

When dx_get_count(entries) == dx_get_limit(entries), ext4_dx_add_entry()
allocates a new index block:

    bh2 = ext4_append(handle, dir, &newblock);
    if (IS_ERR(bh2)) {
        err = PTR_ERR(bh2);
        goto cleanup;
    }
    node2 = (struct dx_node *)(bh2->b_data);
    entries2 = node2->entries;
    memset(&node2->fake, 0, sizeof(struct fake_dirent));

Nothing checks whether bh2 aliases frames[0..level].bh or the leaf block bh:

1. In the !add_level branch, if bh2 aliases (frame - 1)->bh, the memcpy() into
   entries2 overwrites (frame - 1)->entries, and the subsequent call to
   dx_insert_block((frame - 1), hash2, newblock) reads the corrupted count and
   can memmove() out of bounds past the end of (frame - 1)->bh->b_data.

2. In the add_level branch, if bh2 aliases frames[0].bh, the memcpy() from
   entries (offset 32) to entries2 (offset 8) is an overlapping copy within
   frames[0].bh->b_data that overwrites dxroot->info.indirect_levels at offset
   30, increments it, and jumps directly to journal_error / cleanup without
   reaching do_split(), causing dx_release(frames) to loop out of bounds past
   frames[EXT4_HTREE_LEVEL] or leak frame references.

3. In the !add_level branch when restart == 0 and at - entries < icount1, if
   bh2 aliases the leaf block bh, bh->b_data is overwritten with a dx_node
   header and index entries, and bh2 is released without swapping into
   frame->bh, so do_split() then runs on the corrupted leaf buffer without
   detecting the alias.

Could the alias checks be performed when bh2 is allocated during the index
split as well?

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