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

[Kevin Wolf]

Am 04.05.2020 um 16:23 hat Alberto Garcia geschrieben:
> After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is
> extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are
> zeroized.
> 
> The code however does not detect correctly situations when the old and
> the new end of the image are within the same cluster. The problem can
> be reproduced with these steps:
> 
>    qemu-img create -f qcow2 backing.qcow2 1M
>    qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2
>    qemu-img resize --shrink top.qcow2 520k
>    qemu-img resize top.qcow2 567k
> 
> In the last step offset - zero_start causes an integer wraparound.
> 
> Signed-off-by: Alberto Garcia <address@hidden>

Can you add the reproducer to qemu-iotests?

>  block/qcow2.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> v2:
> - Don't call qcow2_cluster_zeroize() if offset == zero_start
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2ba0b17c39..7ca0327995 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4234,15 +4234,20 @@ 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);

I think this is a bit confusing because zero_start implies that this is
the aligned offset where qcow2_cluster_zeroize() would start. At first I
though this wasn't needed at all any more because you already check
offset > zero_start below. So if MIN() makes a difference, the if block
won't be executed anyway.

It would, however, make a difference for calculating the explicit zero
write for the unaligned head:

    uint64_t len = zero_start - old_length;

Maybe it would be easier to understand if we changed only that line?

    uint64_t len = MIN(zero_start, offset) - old_length;

Kevin