RE: [PATCH] fs/read_write.c: Fix a broken signed integer overflow check.

[David Laight <David.Laight@ACULAB.COM>]

From: Ari Sundholm
> Sent: 07 February 2022 12:07
> 
> The function generic_copy_file_checks() checks that the ends of the
> input and output file ranges do not overflow. Unfortunately, there is
> an issue with the check itself.
> 
> Due to the integer promotion rules in C, the expressions
> (pos_in + count) and (pos_out + count) have an unsigned type because
> the count variable has the type uint64_t. Thus, in many cases where we
> should detect signed integer overflow to have occurred (and thus one or
> more of the ranges being invalid), the expressions will instead be
> interpreted as large unsigned integers. This means the check is broken.
> 
> Fix this by explicitly casting the expressions to loff_t.
...
> ---
>  fs/read_write.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 0074afa7ecb3..64166e74adc5 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1431,7 +1431,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 ((loff_t)(pos_in + count) < pos_in ||
> +			(loff_t)(pos_out + count) < pos_out)
>  		return -EOVERFLOW;

Hard to convince myself that is right.
The old code is the standard check for unsigned addition overflow.
The new one is just odd.

If pos_in is guaranteed to be +ve in a signed variable you can check:
	count < (1ull << 63) - pos_in
since the RHS is then guaranteed not to wrap.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)