On Mon, Feb 07, 2022 at 02:58:59PM +0000, 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?
- 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?
Note that we explicitly truncate count so we won't get past the EOF of file_in right below that check and the check in generic_write_check_limits() truncates count so we won't get past ->s_maxbytes on the filesystem we are writing to.
If both source and destination allow arbitrary offsets, we should not fail on copy that crosses from 2^63-1 to 2^63. Your variant will do just that.
It's multiples of 2^64 that we should never attempt to cross, no matter what.
IOW, what values of pos_in, pos_out, count, input file size and output filesystem file size limit do you think should be rejected with -EOVERFLOW here?