Re: [PATCH v3 05/17] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()

[Eric Blake <eblake@redhat.com>]

On 6/23/20 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.05.2020 21:34, Eric Blake wrote:
>> On 5/11/20 12:17 PM, Alberto Garcia wrote:
>>> On Thu 30 Apr 2020 01:10:21 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>>>      compute 'int tail' via % 'int alignment' - safe
>>>
>>>      tail = (offset + bytes) % alignment;
>>>
>>> both are int64_t, no chance of overflow here?
>>
>> Good question - I know several places check that offset+bytes does not 
>> overflow, but did not specifically audit if this one does.  Adding an 
>> assert() in this function may be easier than trying to prove all 
>> callers pass in safe values.
>>
> 
> Hm, it's preexisting, as int64_t + int may overflow as well. Strange, 
> but I don't see overflow check neither in blk_check_byte_request nor in 
> bdrv_check_byte_request. Only discard, which recently dropped call of 
> bdrv_check_byte_request() has this check.

In fact, iotest 197 (see commit 461743390) is an instance of testing for 
a bug where we overflowed INT_MAX due to rounding up to cluster size, 
even with a transaction request smaller than limits.

> 
> I can add a patch for overflow check in blk_check_byte_request and 
> bdrv_check_byte_request.. But what about alignment? There may be 
> requests, for which bytes + offset doesn't overflow, but do overflow 
> after aligning up. Refactor bdrv_pad_request() to return an error if we 
> can't pad request due to overflow?

The only cases where int64_t + int can overflow due to rounding up for 
alignment are when the file size is extremely close to 2^63 bytes 
already.  The easiest fix is to reject opening a file that reports a 
size that would overflow when rounded up to alignment (that is, if size 
 > INT64_MAX - alignment, we should refuse to proceed).  Such images 
will never occur for actual disk images (because that is really a LOT of 
storage), but are possible over things like NBD (in fact, nbdkit has 
intentionally made it easy to provoke boundary testing near 2^63 bytes, 
and is already aware that anything larger than 2^63-512 is problematic 
in qemu).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org