[PATCH v6 12/12] qcow2: protect data reading by host range reference

[Vladimir Sementsov-Ogievskiy]

Similarly to previous commit: host cluster may be discarded and reused
for another cluster or metadata during data read.

This is not as dangerous as write path, we will not corrupt data or
metadata. Still it's bad: guest will probably see data or metadata
which it should not see, so it's a kind of security hole. Let's fix it
too.

Data reading goes through qcow2_get_host_offset(). Let's reference
range returned by this function. Read path differs from write, as we
have to handle compressed cluster descriptor. Also, we should handle
ZERO and UNALLOCATED clusters, for which we have nothing to ref. So, to
keep the whole logic in one place, create qcow2_put_host_offset(),
which should be always called after qcow2_get_host_offset().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h         |  3 +++
 block/qcow2-cluster.c | 38 ++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         | 15 +++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index c40548c4fb..2ac61eccc5 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -926,6 +926,9 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t 
sector_num,
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
                           unsigned int *bytes, uint64_t *host_offset,
                           QCow2SubclusterType *subcluster_type);
+void qcow2_put_host_offset(BlockDriverState *bs,
+                           unsigned int bytes, uint64_t host_offset,
+                           QCow2SubclusterType subcluster_type);
 int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
                             unsigned int *bytes, uint64_t *host_offset,
                             QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 999a739024..126d95b062 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -568,6 +568,10 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
  * Compressed clusters are always processed one by one.
  *
  * Returns 0 on success, -errno in error cases.
+ *
+ * The returned range is referenced, so that it can't be discarded in parallel.
+ * Caller is responsible to unref by qcow2_put_host_offset() after finishing IO
+ * operations with the range.
  */
 int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
                           unsigned int *bytes, uint64_t *host_offset,
@@ -721,6 +725,17 @@ out:
 
     *subcluster_type = type;
 
+    if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
+        uint64_t coffset;
+        int csize;
+
+        qcow2_parse_compressed_cluster_descriptor(s, *host_offset, &coffset,
+                                                  &csize);
+        qcow2_host_range_ref(bs, coffset, csize);
+    } else if (*host_offset) {
+        qcow2_host_range_ref(bs, *host_offset, *bytes);
+    }
+
     return 0;
 
 fail:
@@ -728,6 +743,29 @@ fail:
     return ret;
 }
 
+/*
+ * Caller of qcow2_get_host_offset() must call qcow2_put_host_offset() with
+ * returned parameters of qcow2_get_host_offset() when caller don't need them
+ * anymore.
+ */
+void qcow2_put_host_offset(BlockDriverState *bs,
+                           unsigned int bytes, uint64_t host_offset,
+                           QCow2SubclusterType subcluster_type)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (subcluster_type == QCOW2_SUBCLUSTER_COMPRESSED) {
+        uint64_t coffset;
+        int csize;
+
+        qcow2_parse_compressed_cluster_descriptor(s, host_offset, &coffset,
+                                                  &csize);
+        qcow2_host_range_unref(bs, coffset, csize);
+    } else if (host_offset) {
+        qcow2_host_range_unref(bs, host_offset, bytes);
+    }
+}
+
 /*
  * get_cluster_table
  *
diff --git a/block/qcow2.c b/block/qcow2.c
index d0d2eaa914..d3461b7243 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,6 +2069,8 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
         return ret;
     }
 
+    qcow2_put_host_offset(bs, bytes, host_offset, type);
+
     *pnum = bytes;
 
     if ((type == QCOW2_SUBCLUSTER_NORMAL ||
@@ -2227,6 +2229,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
     return 0;
 }
 
+/* Function consumes host range reference if needed */
 static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
                                              QCow2SubclusterType subc_type,
                                              uint64_t host_offset,
@@ -2272,6 +2275,8 @@ static coroutine_fn int 
qcow2_co_preadv_task(BlockDriverState *bs,
         g_assert_not_reached();
     }
 
+    qcow2_put_host_offset(bs, bytes, host_offset, subc_type);
+
     return ret;
 }
 
@@ -2320,6 +2325,7 @@ static coroutine_fn int 
qcow2_co_preadv_part(BlockDriverState *bs,
             (type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC && !bs->backing))
         {
             qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
+            qcow2_put_host_offset(bs, cur_bytes, host_offset, type);
         } else {
             if (!aio && cur_bytes != bytes) {
                 aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
@@ -3968,6 +3974,12 @@ static coroutine_fn int 
qcow2_co_pwrite_zeroes(BlockDriverState *bs,
             return ret;
         }
 
+        /*
+         * We do the whole thing under s->lock, so we are safe in modifying
+         * metadata. We don't need the reference.
+         */
+        qcow2_put_host_offset(bs, nr, off, type);
+
         if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
             type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC &&
             type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
@@ -4064,6 +4076,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
             break;
 
         case QCOW2_SUBCLUSTER_COMPRESSED:
+            qcow2_put_host_offset(bs, cur_bytes, copy_offset, type);
             ret = -ENOTSUP;
             goto out;
 
@@ -4079,6 +4092,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
                                       copy_offset,
                                       dst, dst_offset,
                                       cur_bytes, read_flags, cur_write_flags);
+        qcow2_put_host_offset(bs, cur_bytes, copy_offset, type);
         qemu_co_mutex_lock(&s->lock);
         if (ret < 0) {
             goto out;
@@ -4700,6 +4714,7 @@ void 
qcow2_parse_compressed_cluster_descriptor(BDRVQcow2State *s,
         (*coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
 }
 
+/* Function consumes host range reference */
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
                            uint64_t cluster_descriptor,
-- 
2.29.2