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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
> Tested-by: syzbot+09bec78ee77613a3efdd@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Adriano Cordova <adrianox@xxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR