From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54296) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBWo-0000E3-HJ for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:26:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKBWh-00068M-Ve for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:26:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKBWh-00068G-NN for qemu-devel@nongnu.org; Wed, 20 Aug 2014 15:26:31 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7KJQU8P030050 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 20 Aug 2014 15:26:31 -0400 Message-ID: <53F4F663.3060707@redhat.com> Date: Wed, 20 Aug 2014 21:26:27 +0200 From: Max Reitz MIME-Version: 1.0 References: <1408223814-23999-1-git-send-email-mreitz@redhat.com> <1408223814-23999-5-git-send-email-mreitz@redhat.com> <20140820105121.GE6122@noname.redhat.com> In-Reply-To: <20140820105121.GE6122@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] qcow2: Check L1/L2/reftable entries for alignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 20.08.2014 12:51, Kevin Wolf wrote: > Am 16.08.2014 um 23:16 hat Max Reitz geschrieben: >> 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 >> --- >> block/qcow2-cluster.c | 27 ++++++++++++++++++++++++++- >> block/qcow2-refcount.c | 36 ++++++++++++++++++++++++++++++++++-- >> 2 files changed, 60 insertions(+), 3 deletions(-) > Can you extend qemu-iotests 060 to check each of these cases? I'll do my very best. >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 5b36018..2cc41b2 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -486,6 +486,12 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, >> goto out; >> } >> >> + if (offset_into_cluster(s, l2_offset)) { >> + qcow2_signal_corruption(bs, -1, -1, "L2 table offset %#" PRIx64 >> + " unaligned", l2_offset); > Should we include l1_index in the message? If you want to debug the > image with a hex editor or something, this is important information. Probably so, yes. >> + return -EIO; >> + } >> + >> /* load the l2 table in memory */ >> >> ret = l2_load(bs, l2_offset, &l2_table); >> @@ -525,6 +531,12 @@ 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, -1, -1, "Data cluster offset %#" PRIx64 >> + " unaligned", *cluster_offset); > The same thing here would be offset. > >> + qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table); >> + return -EIO; >> + } > I wonder whether a goto fail would start to make sense now, zero > clusters in v2 images have the same qcow2_cache_put/return -EIO code. > > And actually, that's a corruption case as well, so we might call > qcow2_signal_corruption() there. I guess writing a fail path would result in more lines of code overall, but deduplicated longer code is probably better than shorter code with duplications, so why not. ;-) >> break; >> default: >> abort(); >> @@ -576,6 +588,11 @@ 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, -1, -1, "L2 table offset %#" PRIx64 >> + " unaligned", l2_offset); > l1_index again. > >> + return -EIO; >> + } >> >> /* seek the l2 table of the given l2 offset */ >> >> @@ -948,6 +965,14 @@ 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, -1, -1, "Data cluster offset %#llx " >> + "unaligned", >> + cluster_offset & L2E_OFFSET_MASK); > Worth adding guest_offset. > >> + ret = -EIO; >> + goto out; >> + } >> + >> if (*host_offset != 0 && !offset_matches) { >> *bytes = 0; >> ret = 0; >> @@ -979,7 +1004,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 0ac1339..fac2963 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -108,6 +108,12 @@ 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, -1, -1, "Refblock offset %#" PRIx64 >> + " unaligned", refcount_block_offset); > Add 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 +187,12 @@ 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, -1, -1, "Refblock offset %#" PRIx64 >> + " unaligned", refcount_block_offset); >> + return -EIO; >> + } > Same here. > >> return load_refcount_block(bs, refcount_block_offset, >> (void**) refcount_block); >> } >> @@ -836,8 +848,13 @@ 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)) { >> + fprintf(stderr, "qcow2: Cannot free unaligned cluster %#llx\n", >> + l2_entry & L2E_OFFSET_MASK); >> + } else { >> + qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK, >> + nb_clusters << s->cluster_bits, type); >> + } > Hm... Why isn't this a corruption like any other? Unconditional > fprintf() is something I don't like a lot. We already do it in qcow2_free_clusters(). I decided not to make it a corruption because we don't lose anything. The entry is corrupted, but we don't need it anymore anyway; it's overwritten with 0 and wherever the cluster might have been meant to be located, it doesn't matter anymore. >> } >> break; >> case QCOW2_CLUSTER_UNALLOCATED: >> @@ -901,6 +918,13 @@ 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, -1, -1, "L2 table offset %#" PRIx64 >> + " unaligned", l2_offset); >> + ret = -EIO; >> + goto fail; >> + } > Add the L1 index (i) to the message. > >> ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, >> (void**) &l2_table); >> if (ret < 0) { >> @@ -933,6 +957,14 @@ 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, -1, -1, "Data cluster " >> + "offset %#llx unaligned", >> + offset & L2E_OFFSET_MASK); > We don't have a single index describing the cluster here, so you might > either just print both L1 and L2 index or calculate a cluster index. The > former is probably easier and even more useful. > >> + ret = -EIO; >> + goto fail; >> + } >> + >> cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >> if (!cluster_index) { >> /* unallocated */ > Kevin Thanks for all of your reviews! Max