[Qemu-devel] [PATCH 0/2] qcow2: Fix qcow2_get_cluster_offset()

[Qemu-devel] [PATCH 0/2] qcow2: Fix qcow2_get_cluster_offset()

[Max Reitz]

Patch 2 fixes a wrong assertion in qcow2_get_cluster_offet(). Patch 1
fixes wrong range limitations I encountered in qemu-io while trying to
break that wrong assertion.

Not CC-ing qemu-stable because these issues were introduced after 2.6.0.


Max Reitz (2):
  qemu-io: Use correct range limitations
  qcow2: Fix qcow2_get_cluster_offset()

 block/qcow2-cluster.c | 16 +++++++++++-----
 qemu-io-cmds.c        | 13 ++++++-------
 2 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.8.3

[Qemu-devel] [PATCH 1/2] qemu-io: Use correct range limitations

[Max Reitz]

create_iovec() has a comment lamenting the lack of SIZE_T_MAX. Since
there actually is a SIZE_MAX, use it.

Two places use INT_MAX for checking the upper bound of a sector count
that is used as an argument for a blk_*() function (blk_discard() and
blk_write_compressed(), respectively). BDRV_REQUEST_MAX_SECTORS should
be used instead.

And finally, do_co_pwrite_zeroes() used to similarly check that the
sector count does not exceed INT_MAX. However, this function is now
backed by blk_co_pwrite_zeroes() which takes bytes as an argument
instead of sectors. Therefore, it should be the byte count that does not
exceed INT_MAX, not the sector count.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-io-cmds.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09e879f..8237c2f 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -389,9 +389,8 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
             goto fail;
         }
 
-        /* should be SIZE_T_MAX, but that doesn't exist */
-        if (len > INT_MAX) {
-            printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX);
+        if (len > SIZE_MAX) {
+            printf("Argument '%s' exceeds maximum size %zu\n", arg, SIZE_MAX);
             goto fail;
         }
 
@@ -479,7 +478,7 @@ static int do_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
         .done   = false,
     };
 
-    if (count >> BDRV_SECTOR_BITS > INT_MAX) {
+    if (count > INT_MAX) {
         return -ERANGE;
     }
 
@@ -500,7 +499,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
 {
     int ret;
 
-    if (count >> 9 > INT_MAX) {
+    if (count >> 9 > BDRV_REQUEST_MAX_SECTORS) {
         return -ERANGE;
     }
 
@@ -1688,9 +1687,9 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         print_cvtnum_err(count, argv[optind]);
         return 0;
-    } else if (count >> BDRV_SECTOR_BITS > INT_MAX) {
+    } else if (count >> BDRV_SECTOR_BITS > BDRV_REQUEST_MAX_SECTORS) {
         printf("length cannot exceed %"PRIu64", given %s\n",
-               (uint64_t)INT_MAX << BDRV_SECTOR_BITS,
+               (uint64_t)BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS,
                argv[optind]);
         return 0;
     }
-- 
2.8.3

Re: [Qemu-devel] [PATCH 1/2] qemu-io: Use correct range limitations

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On 06/20/2016 08:26 AM, Max Reitz wrote:
> create_iovec() has a comment lamenting the lack of SIZE_T_MAX. Since
> there actually is a SIZE_MAX, use it.
> 
> Two places use INT_MAX for checking the upper bound of a sector count
> that is used as an argument for a blk_*() function (blk_discard() and
> blk_write_compressed(), respectively). BDRV_REQUEST_MAX_SECTORS should
> be used instead.
> 
> And finally, do_co_pwrite_zeroes() used to similarly check that the
> sector count does not exceed INT_MAX. However, this function is now
> backed by blk_co_pwrite_zeroes() which takes bytes as an argument
> instead of sectors. Therefore, it should be the byte count that does not
> exceed INT_MAX, not the sector count.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-io-cmds.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 

> @@ -500,7 +499,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
>  {
>      int ret;
>  
> -    if (count >> 9 > INT_MAX) {
> +    if (count >> 9 > BDRV_REQUEST_MAX_SECTORS) {

Worth s/9/BDRV_SECTOR_BITS/ while touching it?  But not a show-stopper
either way.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/2] qcow2: Fix qcow2_get_cluster_offset()

[Max Reitz]

Recently, qcow2_get_cluster_offset() has been changed to work with bytes
instead of sectors. This invalidated some assertions and introduced a
possible integer multiplication overflow.

This could be reproduced using e.g.

$ qemu-img create -f qcow2 -o cluster_size=1M blub.qcow2 8G
Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 encryption=off
cluster_size=1048576 lazy_refcounts=off refcount_bits=16
$ qemu-io -c map blub.qcow2
qemu-io: qemu/block/qcow2-cluster.c:504: qcow2_get_cluster_offset:
Assertion `bytes_needed <= INT_MAX' failed.
[1]    20775 abort (core dumped)  qemu-io -c map foo.qcow2

This patch removes the now wrong assertion, adding comments and more
assertions to prove its correctness (and fixing the overflow which would
become apparent with the original assertion removed).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 893ddf6..b942fb5 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -484,8 +484,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     unsigned int l2_index;
     uint64_t l1_index, l2_offset, *l2_table;
     int l1_bits, c;
-    unsigned int offset_in_cluster, nb_clusters;
-    uint64_t bytes_available, bytes_needed;
+    unsigned int offset_in_cluster;
+    uint64_t bytes_available, bytes_needed, nb_clusters;
     int ret;
 
     offset_in_cluster = offset_into_cluster(s, offset);
@@ -501,7 +501,6 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     if (bytes_needed > bytes_available) {
         bytes_needed = bytes_available;
     }
-    assert(bytes_needed <= INT_MAX);
 
     *cluster_offset = 0;
 
@@ -538,8 +537,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
     *cluster_offset = be64_to_cpu(l2_table[l2_index]);
 
-    /* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */
     nb_clusters = size_to_clusters(s, bytes_needed);
+    /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned
+     * integers; the minimum cluster size is 512, so this assertion is always
+     * true */
+    assert(nb_clusters <= INT_MAX);
 
     ret = qcow2_get_cluster_type(*cluster_offset);
     switch (ret) {
@@ -586,13 +588,17 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
 
     qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
 
-    bytes_available = (c * s->cluster_size);
+    bytes_available = (int64_t)c * s->cluster_size;
 
 out:
     if (bytes_available > bytes_needed) {
         bytes_available = bytes_needed;
     }
 
+    /* bytes_available <= bytes_needed <= *bytes + offset_in_cluster;
+     * subtracting offset_in_cluster will therefore definitely yield something
+     * not exceeding UINT_MAX */
+    assert(bytes_available - offset_in_cluster <= UINT_MAX);
     *bytes = bytes_available - offset_in_cluster;
 
     return ret;
-- 
2.8.3

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] qcow2: Fix qcow2_get_cluster_offset()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On 06/20/2016 08:26 AM, Max Reitz wrote:
> Recently, qcow2_get_cluster_offset() has been changed to work with bytes
> instead of sectors. This invalidated some assertions and introduced a
> possible integer multiplication overflow.
> 
> This could be reproduced using e.g.
> 
> $ qemu-img create -f qcow2 -o cluster_size=1M blub.qcow2 8G
> Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 encryption=off
> cluster_size=1048576 lazy_refcounts=off refcount_bits=16
> $ qemu-io -c map blub.qcow2
> qemu-io: qemu/block/qcow2-cluster.c:504: qcow2_get_cluster_offset:
> Assertion `bytes_needed <= INT_MAX' failed.
> [1]    20775 abort (core dumped)  qemu-io -c map foo.qcow2
> 
> This patch removes the now wrong assertion, adding comments and more
> assertions to prove its correctness (and fixing the overflow which would
> become apparent with the original assertion removed).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/2] qcow2: Fix qcow2_get_cluster_offset()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On 20.06.2016 16:26, Max Reitz wrote:
> Patch 2 fixes a wrong assertion in qcow2_get_cluster_offet(). Patch 1
> fixes wrong range limitations I encountered in qemu-io while trying to
> break that wrong assertion.
> 
> Not CC-ing qemu-stable because these issues were introduced after 2.6.0.
> 
> 
> Max Reitz (2):
>   qemu-io: Use correct range limitations
>   qcow2: Fix qcow2_get_cluster_offset()
> 
>  block/qcow2-cluster.c | 16 +++++++++++-----
>  qemu-io-cmds.c        | 13 ++++++-------
>  2 files changed, 17 insertions(+), 12 deletions(-)

Applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]