Re: [PATCH] ext4: validate dirents before splitting a directory

Re: [PATCH] ext4: validate dirents before splitting a directory

[syzbot]

> #syz test: upstream

I've failed to parse your command.
Did you perhaps forget to provide the branch name, or added an extra ':'?
Please use one of the two supported formats:
1. #syz test
2. #syz test: repo branch-or-commit-hash
Note the lack of ':' in option 1.

> syzbot reported a slab-use-after-free write in do_split() while
> renaming an entry in a directory that is being converted into an
> indexed (htree) directory by make_indexed_dir():
>
>   BUG: KASAN: slab-use-after-free in dx_move_dirents [inline]
>   BUG: KASAN: slab-use-after-free in do_split+0x1241/0x1e90
>   Write of size 90458 at addr ffff88803b38ac6e by task syz.0.17/6003
>
> do_split() builds a map of the leaf's dirents and then moves a subset
> of them to a new block.  dx_move_dirents() trusts map[i].offs and uses
> the rec_len found there as the length of a memset().  With a corrupted
> or crafted directory block, a bogus map entry makes that rec_len
> garbage (here 90464), turning the memset() into an out-of-bounds write
> that can corrupt arbitrary memory.
>
> Validate each entry that is about to be moved with
> ext4_check_dir_entry() before using it, and bail out with
> -EFSCORRUPTED if the entry does not lie inside the block.  This is the
> same class of validation already used elsewhere in the directory code.
>
> This is a filesystem-corruption hardening issue; the crash needs a
> corrupt directory, which is why it is not reachable on a consistent
> filesystem.  The syzbot "introduced by" bisection pointed at an
> unrelated btrfs commit and can be ignored.
>
> Closes: https://syzkaller.appspot.com/bug?extid=09bec78ee77613a3efdd
> ---
>  fs/ext4/namei.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index a6386c1d237f..b2ec222b15e4 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1951,6 +1951,25 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>  		goto journal_error;
>  	}
>  	map -= count;
> +	/*
> +	 * The map is built from the on-disk dirents, so its entries should
> +	 * always refer to valid dirents.  However, if the leaf block is
> +	 * corrupted (e.g. a crafted image), a bogus map entry can make
> +	 * dx_move_dirents() read a rec_len from an arbitrary location and use
> +	 * it as the length of a memset(), writing far out of bounds.  Validate
> +	 * every entry we are about to move before using it.
> +	 */
> +	for (i = 0; i < count; i++) {
> +		unsigned int off = map[i].offs << 2;
> +
> +		if (off > blocksize - sizeof(struct ext4_dir_entry_2) ||
> +		    ext4_check_dir_entry(dir, NULL,
> +					 (struct ext4_dir_entry_2 *)(data1 + off),
> +					 *bh, data1, blocksize, off)) {
> +			err = -EFSCORRUPTED;
> +			goto out;
> +		}
> +	}
>  	dx_sort_map(map, count);
>  	/* Ensure that neither split block is over half full */
>  	size = 0;
> -- 
> 2.51.0
>