[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/5] qcow2: Check L1/L2/reftable entries for
|
From: |
Benoît Canet |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 4/5] qcow2: Check L1/L2/reftable entries for alignment |
|
Date: |
Mon, 8 Sep 2014 16:40:42 +0200 |
|
User-agent: |
Mutt/1.5.23 (2014-03-12) |
The Friday 05 Sep 2014 à 16:07:18 (+0200), Max Reitz wrote :
> Offsets taken from the L1, L2 and refcount tables are generally assumed
> to be correctly aligned. However, this cannot be guaranteed if the image
> has been written to by something different than qemu, thus check all
> offsets taken from these tables for correct cluster alignment.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> block/qcow2-cluster.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> block/qcow2-refcount.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 82 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 735f687..f7dd8c0 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -486,6 +486,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
> goto out;
> }
>
> + if (offset_into_cluster(s, l2_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %#" PRIx64
> + " unaligned (L1 index: %#" PRIx64 ")",
> + l2_offset, l1_index);
> + return -EIO;
This function mix return ret and goto out and there is more of the second.
Can we do ret = -EIO and goto out for consistency ?
bs->drv == NULL after qcow2_signal_corruption so we are not afraid of out
sides effects.
> + }
> +
> /* load the l2 table in memory */
>
> ret = l2_load(bs, l2_offset, &l2_table);
> @@ -508,8 +515,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
> break;
> case QCOW2_CLUSTER_ZERO:
> if (s->qcow_version < 3) {
> - qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
> - return -EIO;
> + qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry
> found"
> + " in pre-v3 image (L2 offset: %#" PRIx64
> + ", L2 index: %#x)", l2_offset, l2_index);
> + ret = -EIO;
> + goto fail;
> }
> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> &l2_table[l2_index], QCOW_OFLAG_ZERO);
> @@ -525,6 +535,14 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> &l2_table[l2_index], QCOW_OFLAG_ZERO);
> *cluster_offset &= L2E_OFFSET_MASK;
> + if (offset_into_cluster(s, *cluster_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset
> %#"
> + PRIx64 " unaligned (L2 offset: %#" PRIx64
> + ", L2 index: %#x)", *cluster_offset,
> + l2_offset, l2_index);
> + ret = -EIO;
> + goto fail;
> + }
> break;
> default:
> abort();
> @@ -541,6 +559,10 @@ out:
> *num = nb_available - index_in_cluster;
>
> return ret;
> +
> +fail:
> + qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
> + return ret;
> }
>
> /*
> @@ -576,6 +598,12 @@ static int get_cluster_table(BlockDriverState *bs,
> uint64_t offset,
>
> assert(l1_index < s->l1_size);
> l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK;
> + if (offset_into_cluster(s, l2_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %#" PRIx64
> + " unaligned (L1 index: %#" PRIx64 ")",
> + l2_offset, l1_index);
> + return -EIO;
> + }
>
> /* seek the l2 table of the given l2 offset */
>
> @@ -948,6 +976,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t
> guest_offset,
> bool offset_matches =
> (cluster_offset & L2E_OFFSET_MASK) == *host_offset;
>
> + if (offset_into_cluster(s, cluster_offset & L2E_OFFSET_MASK)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset "
> + "%#llx unaligned (guest offset: %#"
> PRIx64
> + ")", cluster_offset & L2E_OFFSET_MASK,
> + guest_offset);
> + ret = -EIO;
> + goto out;
> + }
> +
> if (*host_offset != 0 && !offset_matches) {
> *bytes = 0;
> ret = 0;
> @@ -979,7 +1016,7 @@ out:
>
> /* Only return a host offset if we actually made progress. Otherwise we
> * would make requirements for handle_alloc() that it can't fulfill */
> - if (ret) {
> + if (ret > 0) {
> *host_offset = (cluster_offset & L2E_OFFSET_MASK)
> + offset_into_cluster(s, guest_offset);
> }
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index b9d421e..2bcaaf9 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -108,6 +108,13 @@ static int get_refcount(BlockDriverState *bs, int64_t
> cluster_index)
> if (!refcount_block_offset)
> return 0;
>
> + if (offset_into_cluster(s, refcount_block_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Refblock offset %#" PRIx64
> + " unaligned (reftable index: %#" PRIx64 ")",
> + refcount_block_offset, refcount_table_index);
> + return -EIO;
> + }
> +
> ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
> (void**) &refcount_block);
> if (ret < 0) {
> @@ -181,6 +188,14 @@ static int alloc_refcount_block(BlockDriverState *bs,
>
> /* If it's already there, we're done */
> if (refcount_block_offset) {
> + if (offset_into_cluster(s, refcount_block_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Refblock offset
> %#"
> + PRIx64 " unaligned (reftable index: "
> + "%#x)", refcount_block_offset,
> + refcount_table_index);
> + return -EIO;
> + }
> +
> return load_refcount_block(bs, refcount_block_offset,
> (void**) refcount_block);
> }
> @@ -836,8 +851,14 @@ void qcow2_free_any_clusters(BlockDriverState *bs,
> uint64_t l2_entry,
> case QCOW2_CLUSTER_NORMAL:
> case QCOW2_CLUSTER_ZERO:
> if (l2_entry & L2E_OFFSET_MASK) {
> - qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> - nb_clusters << s->cluster_bits, type);
> + if (offset_into_cluster(s, l2_entry & L2E_OFFSET_MASK)) {
> + qcow2_signal_corruption(bs, false, -1, -1,
> + "Cannot free unaligned cluster
> %#llx",
> + l2_entry & L2E_OFFSET_MASK);
> + } else {
> + qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> + nb_clusters << s->cluster_bits, type);
> + }
> }
> break;
> case QCOW2_CLUSTER_UNALLOCATED:
> @@ -901,6 +922,14 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
> old_l2_offset = l2_offset;
> l2_offset &= L1E_OFFSET_MASK;
>
> + if (offset_into_cluster(s, l2_offset)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset
> %#"
> + PRIx64 " unaligned (L1 index: %#x)",
> + l2_offset, i);
> + ret = -EIO;
> + goto fail;
> + }
> +
> ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> (void**) &l2_table);
> if (ret < 0) {
> @@ -933,6 +962,17 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>
> case QCOW2_CLUSTER_NORMAL:
> case QCOW2_CLUSTER_ZERO:
> + if (offset_into_cluster(s, offset &
> L2E_OFFSET_MASK)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Data "
> + "cluster offset %#llx "
> + "unaligned (L2 offset:
> %#"
> + PRIx64 ", L2 index:
> %#x)",
> + offset & L2E_OFFSET_MASK,
> + l2_offset, j);
> + ret = -EIO;
> + goto fail;
> + }
> +
> cluster_index = (offset & L2E_OFFSET_MASK) >>
> s->cluster_bits;
> if (!cluster_index) {
> /* unallocated */
> --
> 2.1.0
>
>