From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33174) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR32W-0004yi-J0 for qemu-devel@nongnu.org; Mon, 08 Sep 2014 13:47:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XR32Q-00073c-DC for qemu-devel@nongnu.org; Mon, 08 Sep 2014 13:47:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29172) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XR32Q-00073P-4O for qemu-devel@nongnu.org; Mon, 08 Sep 2014 13:47:38 -0400 Message-ID: <540DEBB3.2060702@redhat.com> Date: Mon, 08 Sep 2014 19:47:31 +0200 From: Max Reitz MIME-Version: 1.0 References: <1409926039-29044-1-git-send-email-mreitz@redhat.com> <1409926039-29044-5-git-send-email-mreitz@redhat.com> <20140908144041.GF22582@irqsave.net> In-Reply-To: <20140908144041.GF22582@irqsave.net> Content-Type: text/plain; charset=windows-1252; format=flowed 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: =?windows-1252?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 08.09.2014 16:40, Beno=EEt Canet wrote: > The Friday 05 Sep 2014 =E0 16:07:18 (+0200), Max Reitz wrote : >> Offsets taken from the L1, L2 and refcount tables are generally assume= d >> to be correctly aligned. However, this cannot be guaranteed if the ima= ge >> 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 | 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; >> } >> =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 seco= nd. > 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. The "out" label here is for success; that's why I introduced the "fail"=20 label in this series. I could make qcow2_cache_put() in the fail path=20 optional and then use goto fail, though. But this would only increase=20 the code size with no real benefit apparent to me (no code=20 deduplication; and as far as I remember, we have many functions with=20 fail labels which however use a plain "return" before cleaning up is=20 needed). (before this patch, there were two places using "goto out" in this=20 function, both of which were "successes" (cluster found to be=20 unallocated)); and two places using "return -errno", both of which were=20 failures (the first one due to l2_load() failing and the second one due=20 to a zero cluster found in a pre-v3 image)) Max >> + } >> + >> /* 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 e= ntry 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 o= ffset %#" >> + PRIx64 " unaligned (L2 offset: %#= " PRIx64 >> + ", L2 index: %#x)", *cluster_offs= et, >> + 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, ui= nt64_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 o= ffset " >> + "%#llx unaligned (guest offset: %= #" PRIx64 >> + ")", cluster_offset & L2E_OFFSET_= MASK, >> + 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. Other= wise we >> * would make requirements for handle_alloc() that it can't fulf= ill */ >> - 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, int= 64_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: %#" PRIx= 64 ")", >> + refcount_block_offset, refcount_table= _index); >> + return -EIO; >> + } >> + >> ret =3D qcow2_cache_get(bs, s->refcount_block_cache, refcount_bl= ock_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 o= ffset %#" >> + 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 cluste= r %#llx", >> + l2_entry & L2E_OFFSET_MASK); >> + } else { >> + qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK, >> + nb_clusters << s->cluster_bits, t= ype); >> + } >> } >> break; >> case QCOW2_CLUSTER_UNALLOCATED: >> @@ -901,6 +922,14 @@ int qcow2_update_snapshot_refcount(BlockDriverSta= te *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 o= ffset %#" >> + 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(BlockDriverSta= te *bs, >> =20 >> case QCOW2_CLUSTER_NORMAL: >> case QCOW2_CLUSTER_ZERO: >> + if (offset_into_cluster(s, offset & L2E_OFFSE= T_MASK)) { >> + qcow2_signal_corruption(bs, true, -1, -1,= "Data " >> + "cluster offset %= #llx " >> + "unaligned (L2 of= fset: %#" >> + PRIx64 ", L2 inde= x: %#x)", >> + offset & L2E_OFFS= ET_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 >> >>