From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59860) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjtdq-0007bJ-U4 for qemu-devel@nongnu.org; Mon, 12 May 2014 13:04:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wjtdk-0006YY-PB for qemu-devel@nongnu.org; Mon, 12 May 2014 13:03:54 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:48137 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjtdk-0006YO-FN for qemu-devel@nongnu.org; Mon, 12 May 2014 13:03:48 -0400 Date: Mon, 12 May 2014 19:04:22 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140512170422.GJ7858@irqsave.net> References: <1399899851-5641-1-git-send-email-kwolf@redhat.com> <1399899851-5641-5-git-send-email-kwolf@redhat.com> <20140512155024.GG7858@irqsave.net> <20140512164333.GE4371@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140512164333.GE4371@noname.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/5] qcow1: Validate image size (CVE-2014-0223) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com, ppandit@redhat.com The Monday 12 May 2014 =E0 18:43:33 (+0200), Kevin Wolf wrote : > Am 12.05.2014 um 17:50 hat Beno=EEt Canet geschrieben: > > The Monday 12 May 2014 =E0 15:04:10 (+0200), Kevin Wolf wrote : > > > A huge image size could cause s->l1_size to overflow. Make sure tha= t > > > images never require a L1 table larger than what fits in s->l1_size= . > > >=20 > > > This cannot only cause unbounded allocations, but also the allocati= on of > > > a too small L1 table, resulting in out-of-bounds array accesses (bo= th > > > reads and writes). > > >=20 > > > Signed-off-by: Kevin Wolf > > > --- > > > block/qcow.c | 16 ++++++++++++++-- > > > tests/qemu-iotests/092 | 9 +++++++++ > > > tests/qemu-iotests/092.out | 7 +++++++ > > > 3 files changed, 30 insertions(+), 2 deletions(-) > > >=20 > > > diff --git a/block/qcow.c b/block/qcow.c > > > index e8038e5..3566c05 100644 > > > --- a/block/qcow.c > > > +++ b/block/qcow.c > > > @@ -61,7 +61,7 @@ typedef struct BDRVQcowState { > > > int cluster_sectors; > > > int l2_bits; > > > int l2_size; > > > - int l1_size; > > > + unsigned int l1_size; > > > uint64_t cluster_offset_mask; > > > uint64_t l1_table_offset; > > > uint64_t *l1_table; > > > @@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDi= ct *options, int flags, > > > =20 > > > /* read the level 1 table */ > > > shift =3D s->cluster_bits + s->l2_bits; > > > - s->l1_size =3D (header.size + (1LL << shift) - 1) >> shift; > > > + if (header.size > UINT64_MAX - (1LL << shift)) { > >=20 > > I won't be much helpfull but this feel wrong. > > Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cl= uster_bits + s->l2_bits) bytes ? > > Where the size for the L2 chunk themselves is accounted ? >=20 > Not sure what your concern is, but this is basically the same system as > with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map > virtual disk clusters to image file clusters. They don't map metadata > like themselves. >=20 > One cluster contains (1 << cluster_bits) bytes. One L2 table contains > mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits + > l2_bits)) is the number of bytes on the virtual disk that are described > by a single L2 table. I am under the impression that this test compute the maximum size left fo= r the header. So as there is probably more that one L2 table the space left for the hea= der is 1 - nb_l2_table * number_of_byte_covered_by_l2 - number of byte of l1 = - number of=20 bytes of l2 themselve. >=20 > All of this is not related to this patch. All I'm doing here is catchin= g > integer overflows in the calculation of s->l1_size. Apart from error > cases, the calculation is unchanged. >=20 > Kevin >=20 > > > + error_setg(errp, "Image too large"); > > > + ret =3D -EINVAL; > > > + goto fail; > > > + } else { > > > + uint64_t l1_size =3D (header.size + (1LL << shift) - 1) >>= shift; > > > + if (l1_size > INT_MAX / sizeof(uint64_t)) { > > > + error_setg(errp, "Image too large"); > > > + ret =3D -EINVAL; > > > + goto fail; > > > + } > > > + s->l1_size =3D l1_size; > > > + } > > > =20 > > > s->l1_table_offset =3D header.l1_table_offset; > > > s->l1_table =3D g_malloc(s->l1_size * sizeof(uint64_t)); >=20