Re: [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed

[Jan Kara <jack@suse.cz>]

On Tue 02-07-24 21:23:48, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> Syzbot reports a issue as follows:
> ============================================
> BUG: unable to handle page fault for address: ffffed11022e24fe
> PGD 23ffee067 P4D 23ffee067 PUD 0
> Oops: Oops: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 0 PID: 5079 Comm: syz-executor306 Not tainted 6.10.0-rc5-g55027e689933 #0
> Call Trace:
>  <TASK>
>  make_indexed_dir+0xdaf/0x13c0 fs/ext4/namei.c:2341
>  ext4_add_entry+0x222a/0x25d0 fs/ext4/namei.c:2451
>  ext4_rename fs/ext4/namei.c:3936 [inline]
>  ext4_rename2+0x26e5/0x4370 fs/ext4/namei.c:4214
> [...]
> ============================================
> 
> The immediate cause of this problem is that there is only one valid dentry
> for the block to be split during do_split, so split==0 results in out of
> bounds accesses to the map triggering the issue.
> 
>     do_split
>       unsigned split
>       dx_make_map
>        count = 1
>       split = count/2 = 0;
>       continued = hash2 == map[split - 1].hash;
>        ---> map[4294967295]
> 
> The maximum length of a filename is 255 and the minimum block size is 1024,
> so it is always guaranteed that the number of entries is greater than or
> equal to 2 when do_split() is called.
> 
> But syzbot's crafted image has no dot and dotdot in dir, and the dentry
> distribution in dirblock is as follows:
> 
>   bus     dentry1          hole           dentry2           free
> |xx--|xx-------------|...............|xx-------------|...............|
> 0   12 (8+248)=256  268     256     524 (8+256)=264 788     236     1024
> 
> So when renaming dentry1 increases its name_len length by 1, neither hole
> nor free is sufficient to hold the new dentry, and make_indexed_dir() is
> called.
> 
> In make_indexed_dir() it is assumed that the first two entries of the
> dirblock must be dot and dotdot, so bus and dentry1 are left in dx_root
> because they are treated as dot and dotdot, and only dentry2 is moved
> to the new leaf block. That's why count is equal to 1.
> 
> Therefore add the ext4_check_dx_root() helper function to add more sanity
> checks to dot and dotdot before starting the conversion to avoid the above
> issue.
> 
> Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0
> Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> Cc: stable@kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Thanks! The patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/namei.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e6769b97a970..35881e3dd880 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2172,6 +2172,52 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
>  	return err ? err : err2;
>  }
>  
> +static bool ext4_check_dx_root(struct inode *dir, struct dx_root *root)
> +{
> +	struct fake_dirent *fde;
> +	const char *error_msg;
> +	unsigned int rlen;
> +	unsigned int blocksize = dir->i_sb->s_blocksize;
> +	char *blockend = (char *)root + dir->i_sb->s_blocksize;
> +
> +	fde = &root->dot;
> +	if (unlikely(fde->name_len != 1)) {
> +		error_msg = "invalid name_len for '.'";
> +		goto corrupted;
> +	}
> +	if (unlikely(strncmp(root->dot_name, ".", fde->name_len))) {
> +		error_msg = "invalid name for '.'";
> +		goto corrupted;
> +	}
> +	rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
> +	if (unlikely((char *)fde + rlen >= blockend)) {
> +		error_msg = "invalid rec_len for '.'";
> +		goto corrupted;
> +	}
> +
> +	fde = &root->dotdot;
> +	if (unlikely(fde->name_len != 2)) {
> +		error_msg = "invalid name_len for '..'";
> +		goto corrupted;
> +	}
> +	if (unlikely(strncmp(root->dotdot_name, "..", fde->name_len))) {
> +		error_msg = "invalid name for '..'";
> +		goto corrupted;
> +	}
> +	rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
> +	if (unlikely((char *)fde + rlen >= blockend)) {
> +		error_msg = "invalid rec_len for '..'";
> +		goto corrupted;
> +	}
> +
> +	return true;
> +
> +corrupted:
> +	EXT4_ERROR_INODE(dir, "Corrupt dir, %s, running e2fsck is recommended",
> +			 error_msg);
> +	return false;
> +}
> +
>  /*
>   * This converts a one block unindexed directory to a 3 block indexed
>   * directory, and adds the dentry to the indexed directory.
> @@ -2206,17 +2252,17 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>  		brelse(bh);
>  		return retval;
>  	}
> +
>  	root = (struct dx_root *) bh->b_data;
> +	if (!ext4_check_dx_root(dir, root)) {
> +		brelse(bh);
> +		return -EFSCORRUPTED;
> +	}
>  
>  	/* The 0th block becomes the root, move the dirents out */
>  	fde = &root->dotdot;
>  	de = (struct ext4_dir_entry_2 *)((char *)fde +
>  		ext4_rec_len_from_disk(fde->rec_len, blocksize));
> -	if ((char *) de >= (((char *) root) + blocksize)) {
> -		EXT4_ERROR_INODE(dir, "invalid rec_len for '..'");
> -		brelse(bh);
> -		return -EFSCORRUPTED;
> -	}
>  	len = ((char *) root) + (blocksize - csum_size) - (char *) de;
>  
>  	/* Allocate new block for the 0th block's dirents */
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR