Re: [PATCH -next] ext4: fix use-after-free in ext4_rename_dir_prepare

[Jan Kara]

On Thu 14-04-22 10:52:23, Ye Bin wrote:
> We got issue as follows:
> EXT4-fs (loop0): mounted filesystem without journal. Opts: ,errors=continue
> ext4_get_first_dir_block: bh->b_data=0xffff88810bee6000 len=34478
> ext4_get_first_dir_block: *parent_de=0xffff88810beee6ae bh->b_data=0xffff88810bee6000
> ext4_rename_dir_prepare: [1] parent_de=0xffff88810beee6ae
> ==================================================================
> BUG: KASAN: use-after-free in ext4_rename_dir_prepare+0x152/0x220
> Read of size 4 at addr ffff88810beee6ae by task rep/1895
> 
> CPU: 13 PID: 1895 Comm: rep Not tainted 5.10.0+ #241
> Call Trace:
>  dump_stack+0xbe/0xf9
>  print_address_description.constprop.0+0x1e/0x220
>  kasan_report.cold+0x37/0x7f
>  ext4_rename_dir_prepare+0x152/0x220
>  ext4_rename+0xf44/0x1ad0
>  ext4_rename2+0x11c/0x170
>  vfs_rename+0xa84/0x1440
>  do_renameat2+0x683/0x8f0
>  __x64_sys_renameat+0x53/0x60
>  do_syscall_64+0x33/0x40
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f45a6fc41c9
> RSP: 002b:00007ffc5a470218 EFLAGS: 00000246 ORIG_RAX: 0000000000000108
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f45a6fc41c9
> RDX: 0000000000000005 RSI: 0000000020000180 RDI: 0000000000000005
> RBP: 00007ffc5a470240 R08: 00007ffc5a470160 R09: 0000000020000080
> R10: 00000000200001c0 R11: 0000000000000246 R12: 0000000000400bb0
> R13: 00007ffc5a470320 R14: 0000000000000000 R15: 0000000000000000
> 
> The buggy address belongs to the page:
> page:00000000440015ce refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x10beee
> flags: 0x200000000000000()
> raw: 0200000000000000 ffffea00043ff4c8 ffffea0004325608 0000000000000000
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  ffff88810beee580: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>  ffff88810beee600: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> >ffff88810beee680: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>                                   ^
>  ffff88810beee700: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>  ffff88810beee780: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ==================================================================
> Disabling lock debugging due to kernel taint
> ext4_rename_dir_prepare: [2] parent_de->inode=3537895424
> ext4_rename_dir_prepare: [3] dir=0xffff888124170140
> ext4_rename_dir_prepare: [4] ino=2
> ext4_rename_dir_prepare: ent->dir->i_ino=2 parent=-757071872
> 
> Reason is first directory entry which 'rec_len' is 34478, then will get illegal
> parent entry. Now, we do not check directory entry after read directory block
> in 'ext4_get_first_dir_block'.
> To solve this issue, check directory entry in 'ext4_get_first_dir_block'.
> 
> Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e37da8d5cd0c..2f78544b1d47 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3455,6 +3455,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  	struct buffer_head *bh;
>  
>  	if (!ext4_has_inline_data(inode)) {
> +		struct ext4_dir_entry_2 *de;
> +		unsigned int offset;
> +
>  		/* The first directory block must not be a hole, so
>  		 * treat it as DIRENT_HTREE
>  		 */
> @@ -3463,9 +3466,28 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  			*retval = PTR_ERR(bh);
>  			return NULL;
>  		}
> -		*parent_de = ext4_next_entry(
> -					(struct ext4_dir_entry_2 *)bh->b_data,
> -					inode->i_sb->s_blocksize);
> +
> +		de = (struct ext4_dir_entry_2 *) bh->b_data;
> +		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
> +					 bh->b_size, 0) ||
> +		    le32_to_cpu(de->inode) != inode->i_ino ||
> +		    strcmp(".", de->name)) {
> +			ext4_warning_inode(inode, "directory missing '.'");
> +			brelse(bh);
> +			return NULL;
> +		}
> +		offset = ext4_rec_len_from_disk(de->rec_len,
> +						inode->i_sb->s_blocksize);
> +		de = ext4_next_entry(de, inode->i_sb->s_blocksize);
> +		if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data,
> +					 bh->b_size, offset) ||
> +		    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
> +			ext4_warning_inode(inode, "directory missing '..'");
> +			brelse(bh);
> +			return NULL;
> +		}
> +		*parent_de = de;
> +
>  		return bh;
>  	}
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR