Re: [PATCH v3] fs: fix unintentional arithmetic wraparound in offset calculation

[Jan Kara <jack@suse.cz>]

On Fri 17-05-24 00:29:06, Justin Stitt wrote:
> When running syzkaller with the newly reintroduced signed integer
> overflow sanitizer we encounter this report:
> 
> UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10
> 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long')
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x93/0xd0
>  handle_overflow+0x171/0x1b0
>  generic_file_llseek_size+0x35b/0x380
> 
> ... amongst others:
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12
> 142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long')
> ...
> UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11
> 9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long')
> 
> Fix the accidental overflow in these position and offset calculations
> by checking for negative position values, using check_add_overflow()
> helpers and clamping values to expected ranges.
> 
> Link: https://github.com/llvm/llvm-project/pull/82432 [1]
> Closes: https://github.com/KSPP/linux/issues/358
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Except for the unfortunate wording in the changelog, the code actually
looks easier to grasp to me and if it helps the compiler as well, I'm in
favor of this change (but I definitely don't want to overrule Al if he
hates it ;)).

Regarding the code:

> @@ -1467,8 +1470,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
>  
>  	/* Don't allow overlapped copying within the same file. */
>  	if (inode_in == inode_out &&
> -	    pos_out + count > pos_in &&
> -	    pos_out < pos_in + count)
> +	    out_sum > pos_in &&
> +	    pos_out < in_sum)
>  		return -EINVAL;

This is actually subtly wrong becaue 'count' could have been updated
(shrinked) between the check_add_overflow() and this place. So please keep
the old checks here.

> @@ -1649,6 +1652,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)
>  	loff_t max_size = inode->i_sb->s_maxbytes;
>  	loff_t limit = rlimit(RLIMIT_FSIZE);
>  
> +	if (pos < 0)
> +		return -EINVAL;
> +
>  	if (limit != RLIM_INFINITY) {
>  		if (pos >= limit) {
>  			send_sig(SIGXFSZ, current, 0);

Here I'm a bit confused. How is this related to the signed overflow
handling?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR