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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <051a5e4621344301a2c4f84c3de57ec3@AcuMS.aculab.com>
Date:   Mon, 7 Feb 2022 15:50:16 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Ari Sundholm' <ari@...era.com>,
        Andrew Morton <akpm@...ux-foundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Anton Altaparmakov <anton@...era.com>
Subject: RE: [PATCH] fs/read_write.c: Fix a broken signed integer overflow
 check.

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)