From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56385) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR08S-0002ec-Lo for qemu-devel@nongnu.org; Mon, 08 Sep 2014 10:41:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XR08M-0000Ea-GV for qemu-devel@nongnu.org; Mon, 08 Sep 2014 10:41:40 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:60233 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR08M-0000EE-7D for qemu-devel@nongnu.org; Mon, 08 Sep 2014 10:41:34 -0400 Date: Mon, 8 Sep 2014 16:40:42 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140908144041.GF22582@irqsave.net> References: <1409926039-29044-1-git-send-email-mreitz@redhat.com> <1409926039-29044-5-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1409926039-29044-5-git-send-email-mreitz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 4/5] qcow2: Check L1/L2/reftable entries for alignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi The Friday 05 Sep 2014 =E0 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 imag= e > has been written to by something different than qemu, thus check all > offsets taken from these tables for correct cluster alignment. >=20 > Signed-off-by: Max Reitz > --- > block/qcow2-cluster.c | 43 ++++++++++++++++++++++++++++++++++++++++--= - > block/qcow2-refcount.c | 44 ++++++++++++++++++++++++++++++++++++++++++= -- > 2 files changed, 82 insertions(+), 5 deletions(-) >=20 > 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; > } > =20 > + 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 =3D -EIO and goto out for consistency ? bs->drv =3D=3D NULL after qcow2_signal_corruption so we are not afraid of= out sides effects. > + } > + > /* load the l2 table in memory */ > =20 > ret =3D 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 en= try found" > + " in pre-v3 image (L2 offset: %#" = PRIx64 > + ", L2 index: %#x)", l2_offset, l2_= index); > + ret =3D -EIO; > + goto fail; > } > c =3D 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 =3D count_contiguous_clusters(nb_clusters, s->cluster_size, > &l2_table[l2_index], QCOW_OFLAG_ZERO); > *cluster_offset &=3D L2E_OFFSET_MASK; > + if (offset_into_cluster(s, *cluster_offset)) { > + qcow2_signal_corruption(bs, true, -1, -1, "Data cluster of= fset %#" > + PRIx64 " unaligned (L2 offset: %#"= PRIx64 > + ", L2 index: %#x)", *cluster_offse= t, > + l2_offset, l2_index); > + ret =3D -EIO; > + goto fail; > + } > break; > default: > abort(); > @@ -541,6 +559,10 @@ out: > *num =3D nb_available - index_in_cluster; > =20 > return ret; > + > +fail: > + qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table); > + return ret; > } > =20 > /* > @@ -576,6 +598,12 @@ static int get_cluster_table(BlockDriverState *bs,= uint64_t offset, > =20 > assert(l1_index < s->l1_size); > l2_offset =3D 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; > + } > =20 > /* seek the l2 table of the given l2 offset */ > =20 > @@ -948,6 +976,15 @@ static int handle_copied(BlockDriverState *bs, uin= t64_t guest_offset, > bool offset_matches =3D > (cluster_offset & L2E_OFFSET_MASK) =3D=3D *host_offset; > =20 > + if (offset_into_cluster(s, cluster_offset & L2E_OFFSET_MASK)) = { > + qcow2_signal_corruption(bs, true, -1, -1, "Data cluster of= fset " > + "%#llx unaligned (guest offset: %#= " PRIx64 > + ")", cluster_offset & L2E_OFFSET_M= ASK, > + guest_offset); > + ret =3D -EIO; > + goto out; > + } > + > if (*host_offset !=3D 0 && !offset_matches) { > *bytes =3D 0; > ret =3D 0; > @@ -979,7 +1016,7 @@ out: > =20 > /* Only return a host offset if we actually made progress. Otherwi= se we > * would make requirements for handle_alloc() that it can't fulfil= l */ > - if (ret) { > + if (ret > 0) { > *host_offset =3D (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, int6= 4_t cluster_index) > if (!refcount_block_offset) > return 0; > =20 > + if (offset_into_cluster(s, refcount_block_offset)) { > + qcow2_signal_corruption(bs, true, -1, -1, "Refblock offset %#"= PRIx64 > + " unaligned (reftable index: %#" PRIx6= 4 ")", > + refcount_block_offset, refcount_table_= index); > + return -EIO; > + } > + > ret =3D qcow2_cache_get(bs, s->refcount_block_cache, refcount_bloc= k_offset, > (void**) &refcount_block); > if (ret < 0) { > @@ -181,6 +188,14 @@ static int alloc_refcount_block(BlockDriverState *= bs, > =20 > /* 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 of= fset %#" > + PRIx64 " unaligned (reftable i= ndex: " > + "%#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, ty= pe); > + } > } > break; > case QCOW2_CLUSTER_UNALLOCATED: > @@ -901,6 +922,14 @@ int qcow2_update_snapshot_refcount(BlockDriverStat= e *bs, > old_l2_offset =3D l2_offset; > l2_offset &=3D L1E_OFFSET_MASK; > =20 > + if (offset_into_cluster(s, l2_offset)) { > + qcow2_signal_corruption(bs, true, -1, -1, "L2 table of= fset %#" > + PRIx64 " unaligned (L1 index: = %#x)", > + l2_offset, i); > + ret =3D -EIO; > + goto fail; > + } > + > ret =3D 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(BlockDriverStat= e *bs, > =20 > 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 off= set: %#" > + PRIx64 ", L2 index= : %#x)", > + offset & L2E_OFFSE= T_MASK, > + l2_offset, j); > + ret =3D -EIO; > + goto fail; > + } > + > cluster_index =3D (offset & L2E_OFFSET_MASK) >= > s->cluster_bits; > if (!cluster_index) { > /* unallocated */ > --=20 > 2.1.0 >=20 >=20