Re: [PATCH] ext4: wipe moved dirents with their real length

[Jan Kara]

On Thu 01-10-26 21:41:27, Adriano Cordova wrote:
> After copying a dirent to the new block, dx_move_dirents() wipes the
> source entry using the on-disk rec_len.  Use the length already
> computed for the copy instead because the on-disk rec_len is untrusted
> and a corrupt value makes the memset() run out of bounds.
> 
> 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>

Not that this would be wrong per se but the explanation doesn't really
satisfy me :) If the rec_len on disk is wrong, we have a larger problem
with corrupted source directory and we shouldn't have reached
dx_move_dirents() in the first place. So how come directory entry
validation in dx_make_map() didn't catch the corruption? Did syzkaller
somehow managed to corrupt the directory entry between dx_make_map() and
dx_move_dirents()? If yes, that is the bug that needs fixing, not trying to
suck up corrupted directory entries in dx_move_dirents()...

								Honza

> ---
>  fs/ext4/namei.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index a6386c1d237f..71e9e7c9a7fd 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1860,10 +1860,8 @@ dx_move_dirents(struct inode *dir, char *from, char *to,
>  
>  		/* wipe dir_entry excluding the rec_len field */
>  		de->inode = 0;
> -		memset(&de->name_len, 0, ext4_rec_len_from_disk(de->rec_len,
> -								blocksize) -
> -					 offsetof(struct ext4_dir_entry_2,
> -								name_len));
> +		memset(&de->name_len, 0, rec_len -
> +		       offsetof(struct ext4_dir_entry_2, name_len));
>  
>  		map++;
>  		to += rec_len;
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR