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

[Eric Blake]

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