Re: [PATCH 2/3] vfs: Fix implicit conversion problem when testing overflow case

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Sep 20, 2024 at 08:28:51PM +0800, Julian Sun wrote:
> The overflow check in generic_copy_file_checks() and generic_remap_checks()
> is now broken because the result of the addition is implicitly converted to
> an unsigned type, which disrupts the comparison with signed numbers.
> This caused the kernel to not return EOVERFLOW in copy_file_range()
> call with len is set to 0xffffffffa003e45bul.
> 
> Use the check_add_overflow() macro to fix this issue.
> 
> Reported-and-tested-by: syzbot+296b1c84b9cbf306e5a0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=296b1c84b9cbf306e5a0
> Fixes: 1383a7ed6749 ("vfs: check file ranges before cloning files")
> Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range")
> Inspired-by: Dave Chinner <david@fromorbit.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/read_write.c  | 5 +++--
>  fs/remap_range.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 070a7c33b9dd..5211246edc2e 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1509,7 +1509,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  	struct inode *inode_in = file_inode(file_in);
>  	struct inode *inode_out = file_inode(file_out);
>  	uint64_t count = *req_count;
> -	loff_t size_in;
> +	loff_t size_in, tmp;
>  	int ret;
>  
>  	ret = generic_file_rw_checks(file_in, file_out);
> @@ -1544,7 +1544,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  		return -ETXTBSY;
>  
>  	/* Ensure offsets don't wrap. */
> -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> +	if (check_add_overflow(pos_in, count, &tmp) ||
> +	    check_add_overflow(pos_out, count, &tmp))
>  		return -EOVERFLOW;
>  
>  	/* Shorten the copy to EOF */
> diff --git a/fs/remap_range.c b/fs/remap_range.c
> index 28246dfc8485..6fdeb3c8cb70 100644
> --- a/fs/remap_range.c
> +++ b/fs/remap_range.c
> @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
>  	struct inode *inode_out = file_out->f_mapping->host;
>  	uint64_t count = *req_count;
>  	uint64_t bcount;
> -	loff_t size_in, size_out;
> +	loff_t size_in, size_out, tmp;
>  	loff_t bs = inode_out->i_sb->s_blocksize;
>  	int ret;
>  
> @@ -45,7 +45,8 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
>  		return -EINVAL;
>  
>  	/* Ensure offsets don't wrap. */
> -	if (pos_in + count < pos_in || pos_out + count < pos_out)
> +	if (check_add_overflow(pos_in, count, &tmp) ||
> +	    check_add_overflow(pos_out, count, &tmp))
>  		return -EINVAL;
>  
>  	size_in = i_size_read(inode_in);
> -- 
> 2.39.2
> 
>