Hello, Al,
On 2/7/22 16:58, Al Viro wrote:
On Mon, Feb 07, 2022 at 02:07:11PM +0200, Ari Sundholm wrote:
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.
I must be slow this morning, but... which values of pos_in and count are
caught by your check, but not by the original?
Thank you for your response and questions.
Assuming an x86-64 target platform, please consider:
loff_t pos_out = 0x7FFFFFFFFFFEFFFFLL;
and
uint64_t count = 65537;
The type of the expression (pos_out + count) is a 64-bit unsigned type,
by C's integer promotion rules. Its value is 0x8000000000000000ULL, that
is, bit 63 is set.
The comparison (pos_out + count) < pos_out, again due to C's integer
promotion rules, is unsigned. Thus, the comparison, in this case, is
equivalent to:
0x8000000000000000ULL < 0x7FFFFFFFFFFEFFFFULL,
which is false. Please note that the LHS is not expressible as a
positive integer of type loff_t. With larger values for count, the
problem should become quite obvious, as some the offsets within the file
would not be expressible as positive integers of type loff_t. But I
digress. As we can see above, the overflow is missed.
With the LHS explicitly cast to loff_t, the comparison is equivalent to:
0x8000000000000000LL < 0x7FFFFFFFFFFEFFFFLL,
which is true, as the LHS is negative.
This has also been verified in practice, and was detected when running
tests on special cases of the copy_file_range syscall on different
filesystems.
- 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)
Example, please. Why do you need that comparison to be signed?
Please see the above.
I also created a small test program one can try on Compiler Explorer:
https://godbolt.org/z/e76rb3Ec9
Please let me know if there are any further concerns.
Best regards,
Ari Sundholm
ari@xxxxxxxxxx
