[2/2] qcow2: Fix qcow2_get_cluster_offset()
diff mbox

Message ID 20160620142623.24471-3-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz June 20, 2016, 2:26 p.m. UTC
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(-)

Comments

Eric Blake June 20, 2016, 3:31 p.m. UTC | #1
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>

Patch
diff mbox

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;