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

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

[Alberto Garcia]

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 <berto@igalia.com>
---
 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);
+
         /*
          * 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);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Failed to zero out new clusters");
-            goto fail;
+        if (offset > zero_start) {
+            ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "Failed to zero out new clusters");
+                goto fail;
+            }
         }
 
         /* Write explicit zeros for the unaligned head */
-- 
2.20.1

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 <berto@igalia.com>

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

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200504142308.10446-1-berto@igalia.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200504142308.10446-1-berto@igalia.com
Subject: [PATCH v2] qcow2: Avoid integer wraparound in qcow2_co_truncate()
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: unable to write new index file
Traceback (most recent call last):
  File "patchew-tester/src/patchew-cli", line 521, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester/src/patchew-cli", line 57, in git_clone_repo
    cwd=clone)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'refs/tags/patchew/20200504142308.10446-1-berto@igalia.com', '-b', 'test']' returned non-zero exit status 128.



The full log is available at
http://patchew.org/logs/20200504142308.10446-1-berto@igalia.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com