[PATCH v2] ext4: don't append a directory block already mapped in the inode

[PATCH v2] ext4: don't append a directory block already mapped in the inode

[Adriano Cordova]

ext4_append() grows a directory by one block.  It checks that the target
logical block is a hole, but a corrupt block bitmap can still make the
allocator hand back a physical block that is already in use by this
inode.  The in-memory copy of a block is keyed by its physical block
number, so the "new" block and that existing one are the same memory;
callers that split a directory (make_indexed_dir()/do_split()) then move
entries between two aliased buffers and corrupt the directory, until a
bogus rec_len read from the middle of a name runs the wipe out of bounds:

  BUG: KASAN: slab-use-after-free in dx_move_dirents [inline]
  Write of size 90458 ...

Reject the block and report the corrupt bitmap instead of corrupting
memory.

Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
Tested-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
Changes in v2:
- Rework after review of v1 ("ext4: wipe moved dirents with their real
  length"). dx_make_map() already validates each entry, so the bad
  rec_len does not come from disk. The problem is that the block
  allocator was handing back a block already mapped by the inode.

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

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3b9740c1c16d..ff6013306b74 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -83,6 +83,25 @@ static struct buffer_head *ext4_append(handle_t *handle,
 	bh = ext4_bread(handle, inode, *block, EXT4_GET_BLOCKS_CREATE);
 	if (IS_ERR(bh))
 		return bh;
+
+	for (map.m_lblk = 0; map.m_lblk < *block; map.m_lblk += map.m_len) {
+		map.m_len = *block - map.m_lblk;
+		err = ext4_map_blocks(NULL, inode, &map, 0);
+		if (err < 0)
+			goto out;
+		if (err == 0) {
+			map.m_len = 1;
+			continue;
+		}
+		if (unlikely(map.m_pblk == bh->b_blocknr)) {
+			EXT4_ERROR_INODE(inode,
+					 "new block %llu already mapped",
+					 (unsigned long long)bh->b_blocknr);
+			err = -EFSCORRUPTED;
+			goto out;
+		}
+	}
+
 	inode->i_size += inode->i_sb->s_blocksize;
 	EXT4_I(inode)->i_disksize = inode->i_size;
 	err = ext4_mark_inode_dirty(handle, inode);
-- 
2.51.0

Re: [PATCH v2] ext4: don't append a directory block already mapped in the inode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ext4: ext4_append() misses aliases in multi-block extents
- [High] ext4: ext4_append() forces O(N) iteration over holes leading to soft lockup
- [High] ext4: ext4_append() escalates transient errors to filesystem corruption
--

commit 4e22a7e845c3a3e2ac2696b510b59d9b11640408
Author: Adriano Cordova <adrianox@gmail.com>
ext4: don't append a directory block already mapped in the inode

This commit adds a check in ext4_append to ensure a newly allocated
directory block is not already mapped by the inode. It iterates through
the inode's logical blocks to verify the new physical block does not
alias an existing one.

Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
[ ... ]
> @@ -83,6 +83,25 @@ static struct buffer_head *ext4_append(handle_t *handle,
>  	bh = ext4_bread(handle, inode, *block, EXT4_GET_BLOCKS_CREATE);
>  	if (IS_ERR(bh))
>  		return bh;
> +
> +	for (map.m_lblk = 0; map.m_lblk < *block; map.m_lblk += map.m_len) {
> +		map.m_len = *block - map.m_lblk;
> +		err = ext4_map_blocks(NULL, inode, &map, 0);
> +		if (err < 0)
> +			goto out;

[Severity: High]
Could this cause a regression where transient errors are escalated to
filesystem corruption?

The goto out path calls ext4_std_error(inode->i_sb, err), which marks the
filesystem as corrupted.

If ext4_map_blocks() returns a transient error like -ENOMEM or -EIO, should
it instead propagate the error directly back to the caller, similar to how
the earlier ext4_map_blocks() call handles them before the loop?

> +		if (err == 0) {
> +			map.m_len = 1;
> +			continue;
> +		}

[Severity: High]
Does this force an iteration over large holes, potentially causing a
performance regression?

When ext4_map_blocks() returns 0 for a hole, it populates map.m_len with
the full length of the hole.

Overriding map.m_len to 1 discards the hole size and forces the loop to
iterate block-by-block. For directories with large sparse holes, could this
lead to severe performance degradation and soft lockups?

> +		if (unlikely(map.m_pblk == bh->b_blocknr)) {

[Severity: High]
Can this equality check miss aliases if the mapped extent spans multiple
blocks, allowing a memory corruption regression?

Because ext4_map_blocks() can return a multi-block extent with map.m_len > 1,
the newly allocated block bh->b_blocknr might fall in the middle of a
previously allocated multi-block extent.

If bh->b_blocknr is within the range from map.m_pblk to
map.m_pblk + map.m_len - 1, this exact match check will evaluate to false,
failing to prevent the alias.

Should this check account for the extent length?

> +			EXT4_ERROR_INODE(inode,
> +					 "new block %llu already mapped",
> +					 (unsigned long long)bh->b_blocknr);
> +			err = -EFSCORRUPTED;
> +			goto out;
> +		}
> +	}
> +
>  	inode->i_size += inode->i_sb->s_blocksize;
>  	EXT4_I(inode)->i_disksize = inode->i_size;
>  	err = ext4_mark_inode_dirty(handle, inode);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005003528.92046-1-adrianox@gmail.com?part=1

Re: [PATCH v2] ext4: don't append a directory block already mapped in the inode

[Jan Kara]

Hello!

On Sun 04-10-26 21:35:28, Adriano Cordova wrote:
> ext4_append() grows a directory by one block.  It checks that the target
> logical block is a hole, but a corrupt block bitmap can still make the
> allocator hand back a physical block that is already in use by this
> inode.  The in-memory copy of a block is keyed by its physical block
> number, so the "new" block and that existing one are the same memory;
> callers that split a directory (make_indexed_dir()/do_split()) then move
> entries between two aliased buffers and corrupt the directory, until a
> bogus rec_len read from the middle of a name runs the wipe out of bounds:
> 
>   BUG: KASAN: slab-use-after-free in dx_move_dirents [inline]
>   Write of size 90458 ...
> 
> Reject the block and report the corrupt bitmap instead of corrupting
> memory.
> 
> Reported-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
> Tested-by: syzbot+09bec78ee77613a3efdd@syzkaller.appspotmail.com
> Signed-off-by: Adriano Cordova <adrianox@gmail.com>

Good that you tracked down the reason for the corruption. But what you do
below isn't really a good fix and I don't think you've put too much thought
into it. Firstly, it would work only in the specific case this syzkaller
reproducer triggers where the same block is claimed twice by the same inode
(but other inodes can end up claiming the block as well!). Secondly, it
would heavily slow down appending to large directories.

Frankly, I don't think this case of multiply claimed blocks is easy to deal
with. To properly solve it you would need something like storing in the
struct buffer_head the type of metadata (and perhaps inode & offset owning
it) when loading metadata from the disk and then validating this when
getting the buffer head from cache. But it's a lot of work and practically
only to make fuzzer of disk images happy so I'm not really sure it's worth
it.

								Honza


> ---
> Changes in v2:
> - Rework after review of v1 ("ext4: wipe moved dirents with their real
>   length"). dx_make_map() already validates each entry, so the bad
>   rec_len does not come from disk. The problem is that the block
>   allocator was handing back a block already mapped by the inode.
> 
>  fs/ext4/namei.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 3b9740c1c16d..ff6013306b74 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -83,6 +83,25 @@ static struct buffer_head *ext4_append(handle_t *handle,
>  	bh = ext4_bread(handle, inode, *block, EXT4_GET_BLOCKS_CREATE);
>  	if (IS_ERR(bh))
>  		return bh;
> +
> +	for (map.m_lblk = 0; map.m_lblk < *block; map.m_lblk += map.m_len) {
> +		map.m_len = *block - map.m_lblk;
> +		err = ext4_map_blocks(NULL, inode, &map, 0);
> +		if (err < 0)
> +			goto out;
> +		if (err == 0) {
> +			map.m_len = 1;
> +			continue;
> +		}
> +		if (unlikely(map.m_pblk == bh->b_blocknr)) {
> +			EXT4_ERROR_INODE(inode,
> +					 "new block %llu already mapped",
> +					 (unsigned long long)bh->b_blocknr);
> +			err = -EFSCORRUPTED;
> +			goto out;
> +		}
> +	}
> +
>  	inode->i_size += inode->i_sb->s_blocksize;
>  	EXT4_I(inode)->i_disksize = inode->i_size;
>  	err = ext4_mark_inode_dirty(handle, inode);
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR