From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsbEZ-00041z-5y for qemu-devel@nongnu.org; Fri, 07 Oct 2016 15:55:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsbEX-0006d5-1N for qemu-devel@nongnu.org; Fri, 07 Oct 2016 15:55:06 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-14-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: <54a9ba09-badf-28f4-2e12-4ff4074072cf@redhat.com> Date: Fri, 7 Oct 2016 21:54:54 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-14-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pnIcg3Q6PTL8mf5ShjLGV82XJrcSRlvm6" Subject: Re: [Qemu-devel] [PATCH 13/22] qcow2-bitmap: check constraints List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --pnIcg3Q6PTL8mf5ShjLGV82XJrcSRlvm6 From: Max Reitz To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Message-ID: <54a9ba09-badf-28f4-2e12-4ff4074072cf@redhat.com> Subject: Re: [PATCH 13/22] qcow2-bitmap: check constraints References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-14-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-14-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Check bitmap header constraints as specified in docs/specs/qcow2.txt >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-bitmap.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) I'd pull this patch to some previous point in the series because the previous patches would already require you to check these constraints (which you just haven't done until now). > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 8cf40f0..1c3abea 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -154,6 +154,34 @@ static inline void bitmap_directory_to_be(uint8_t = *dir, size_t size) > } > } > =20 > +static int check_constraints(BlockDriverState *bs, Qcow2BitmapDirEntry= *h) > +{ > + BDRVQcow2State *s =3D bs->opaque; > + uint64_t phys_bitmap_bytes =3D > + (uint64_t)h->bitmap_table_size * s->cluster_size; > + uint64_t max_virtual_bits =3D (phys_bitmap_bytes * 8) << h->granul= arity_bits; > + int64_t nb_sectors =3D bdrv_nb_sectors(bs); > + > + if (nb_sectors < 0) { > + return nb_sectors; > + } > + > + int fail =3D > + ((h->bitmap_table_size =3D=3D 0) !=3D (h->bitmap_table_off= set =3D=3D 0)) || > + (h->bitmap_table_offset % s->cluster_size) || > + (h->bitmap_table_size > BME_MAX_TABLE_SIZE) || > + (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) || > + (h->bitmap_table_offset !=3D 0 && > + (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) |= | > + (h->granularity_bits > BME_MAX_GRANULARITY_BITS) || > + (h->granularity_bits < BME_MIN_GRANULARITY_BITS) || > + (h->flags & BME_RESERVED_FLAGS) || > + (h->name_size > BME_MAX_NAME_SIZE) || > + (h->type !=3D BT_DIRTY_TRACKING_BITMAP); > + > + return fail ? -EINVAL : 0; > +} > + > static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_= table, > uint32_t bitmap_table_size) > { > @@ -372,6 +400,12 @@ static uint8_t *directory_read(BlockDriverState *b= s, > bdrv_get_device_or_node_name(bs)); > goto fail; > } > + > + ret =3D check_constraints(bs, e); > + if (ret < 0) { > + error_setg(errp, "Bitmap doesn't satisfy the constraints."= ); I think I'd at least mention the name of the bitmap; also, no full stop at the end of error messages. > + goto fail; > + } > } > =20 > assert((uint8_t *)e =3D=3D dir_end); > @@ -713,6 +747,11 @@ static int store_bitmap(BlockDriverState *bs, > entry->extra_data_size =3D 0; > memcpy(entry + 1, bm_name, entry->name_size); > =20 > + ret =3D check_constraints(bs, entry); > + if (ret < 0) { > + goto fail; > + } > + As I said in my second reply to patch 9, I think it's a bit too late if we detect that the bitmap is actually invalid at this point. We really should notice earlier. Apart from what would actually better for the user, it is actually too late to check the constraints here, as you have already written the bitmap data to disk. You should always check the constraints before reading and also before writing, not afterwards. Max > return 0; > =20 > fail: >=20 --pnIcg3Q6PTL8mf5ShjLGV82XJrcSRlvm6 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX9/2PEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQExW CACP2iBC8lwJKs9aBwBDx9658uPO9/rIgXc3zdpAzR1Wbq6Pf26fPl3lWheYNthN c/HIFIRj9BeJ9nMMgI8qFQOkLjpJecA9KYyuqrOb/mtAckoWVtf4sBPPmhlCFoVf SwiD9QWQRyHVxswSDS05qzn9coNbCojcrSal+Q6Gijl+JrJhAm8fSBcYd/ZpCy0J kPIEthC6OhbU9c8eRjFHYP52EVexx43L0HohmN2U1xwvwgS7MKEAB7CDhj6+9mJg EbtpY5dhMa8HstWwzGNduRfdqAtOb1kgBpE/UJp3iSxxW5sHbt45q0XrUcCJAamN bNHmMEoNEQhK+BU9QPVZWrOI =4jEF -----END PGP SIGNATURE----- --pnIcg3Q6PTL8mf5ShjLGV82XJrcSRlvm6--