Re: [PATCH] qcow2: Avoid integer wraparound in qcow2_co_truncate()

[Alberto Garcia]

On Fri 01 May 2020 08:48:31 PM CEST, Eric Blake wrote:
> Since your reproducer triggers assertion failure, I suggest doing this
> instead:

>>> +++ b/block/qcow2.c
>>> @@ -4234,6 +4234,9 @@ static int coroutine_fn 
>>> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>       if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) {
>>>           uint64_t zero_start = QEMU_ALIGN_UP(old_length, 
>>> s->cluster_size);
>>> +        /* zero_start should not be after the new end of the image */
>>> +        zero_start = MIN(zero_start, offset);
>>> +
>
> Drop this hunk (leave zero_start unchanged), and instead...
>
>> 
>> So, using your numbers, pre-patch, we have zero_start = 0x90000 (0x82000 
>> rounded up to 0x10000 alignment).  post-patch, the new MIN() lowers it 
>> back to 0x8dc00 (the new size), which is unaligned.
>> 
>>>           /*
>>>            * Use zero clusters as much as we can. qcow2_cluster_zeroize()
>>>            * requires a cluster-aligned start. The end may be 
>>> unaligned if it is
>>           * at the end of the image (which it is here).
>>           */
>>          ret = qcow2_cluster_zeroize(bs, zero_start, offset - 
>> zero_start, 0);
>
> ...patch _this_ call to compute 'QEMU_ALIGN_UP(offset, s->cluster_size) 
> - zero_start' for the length.

That would work, but then we would be writing zeroes beyond the end of
the image (but still within the last cluster).

The other solution is to keep my hunk and call qcow2_cluster_zeroize()
only when offset > zero_start.

Berto