From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2Mlh-00017G-Ja for qemu-devel@nongnu.org; Tue, 09 Jun 2015 12:52:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2Mle-00038U-9A for qemu-devel@nongnu.org; Tue, 09 Jun 2015 12:52:53 -0400 Received: from mail-wg0-x22c.google.com ([2a00:1450:400c:c00::22c]:35913) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2Mle-000389-2Y for qemu-devel@nongnu.org; Tue, 09 Jun 2015 12:52:50 -0400 Received: by wgbgq6 with SMTP id gq6so18174361wgb.3 for ; Tue, 09 Jun 2015 09:52:49 -0700 (PDT) Date: Tue, 9 Jun 2015 17:52:45 +0100 From: Stefan Hajnoczi Message-ID: <20150609165245.GH3181@stefanha-thinkpad.redhat.com> References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-3-git-send-email-vsementsov@virtuozzo.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AqCDj3hiknadvR6t" Content-Disposition: inline In-Reply-To: <1433776886-27239-3-git-send-email-vsementsov@virtuozzo.com> Subject: Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy Cc: kwolf@redhat.com, qemu-devel@nongnu.org, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com, jsnow@redhat.com --AqCDj3hiknadvR6t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 08, 2015 at 06:21:20PM +0300, Vladimir Sementsov-Ogievskiy wrot= e: I haven't fully reviewed this patch yet but here are initial comments. > +int qcow2_read_dirty_bitmaps(BlockDriverState *bs) > +{ > + BDRVQcowState *s =3D bs->opaque; > + QCowDirtyBitmapHeader h; > + QCowDirtyBitmap *bm; > + int i, name_size; > + int64_t offset; > + int ret; > + > + if (!s->nb_dirty_bitmaps) { > + s->dirty_bitmaps =3D NULL; > + s->dirty_bitmaps_size =3D 0; > + return 0; > + } > + > + offset =3D s->dirty_bitmaps_offset; > + s->dirty_bitmaps =3D g_new0(QCowDirtyBitmap, s->nb_dirty_bitmaps); > + > + for (i =3D 0; i < s->nb_dirty_bitmaps; i++) { > + /* Read statically sized part of the dirty_bitmap header */ > + offset =3D align_offset(offset, 8); > + ret =3D bdrv_pread(bs->file, offset, &h, sizeof(h)); > + if (ret < 0) { > + goto fail; > + } > + > + offset +=3D sizeof(h); > + bm =3D s->dirty_bitmaps + i; > + bm->l1_table_offset =3D be64_to_cpu(h.l1_table_offset); > + bm->l1_size =3D be32_to_cpu(h.l1_size); > + bm->bitmap_granularity =3D be32_to_cpu(h.bitmap_granularity); > + bm->bitmap_size =3D be64_to_cpu(h.bitmap_size); Input validation is missing. These could be junk values. Min, max, alignment, etc need to be checked. > @@ -90,6 +91,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, = uint64_t start_offset, > QCowExtension ext; > uint64_t offset; > int ret; > + Qcow2DirtyBitmapHeaderExt dirty_bitmaps_ext; > =20 > #ifdef DEBUG_EXT > printf("qcow2_read_extensions: start=3D%ld end=3D%ld\n", start_offse= t, end_offset); > @@ -160,6 +162,33 @@ static int qcow2_read_extensions(BlockDriverState *b= s, uint64_t start_offset, > } > break; > =20 > + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: > + ret =3D bdrv_pread(bs->file, offset, &dirty_bitmaps_ext, ext= =2Elen); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "ERROR: dirty_bitmaps_ext: " > + "Could not read ext header"); > + return ret; > + } > + > + be64_to_cpus(&dirty_bitmaps_ext.dirty_bitmaps_offset); > + be32_to_cpus(&dirty_bitmaps_ext.nb_dirty_bitmaps); > + > + s->dirty_bitmaps_offset =3D dirty_bitmaps_ext.dirty_bitmaps_= offset; > + s->nb_dirty_bitmaps =3D dirty_bitmaps_ext.nb_dirty_bitmaps; > + > + ret =3D qcow2_read_dirty_bitmaps(bs); Missing input validation. We cannot trust dirty_bitmaps_offset or nb_dirty_bitmaps. --AqCDj3hiknadvR6t Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVdxndAAoJEJykq7OBq3PIPfcH/jOFyD30zWFLQJKPNSVwzjsd wFsU6T9wpHwIUEcqDnii0wOuzi6AggiWFs8locyiXyfbZh3kOqR5pMaenMJOYZBQ /+beiUBgK5TXmFC1WOpBYqYMjOt1YFnPz1cgCVRPXCp54lh9I6yBjXp/korqR86x uJ7ZnNesB1kr5GFREjK9Zi5eR+senQXN3oNL8Pa3apDyXj8BvthIWMGME525ahrX XdhImANg2dD5zKBfp57+Zlm6p1XKmFESyUmhU6wB62CoOxfLg35yMfho2J0GCiNb WUYDtgzv8y2ZKdXM/Dk4qw0h36a/IlYpZmwUFgKT5C3TXl70JzWOm9l0kJsqZ+k= =laLP -----END PGP SIGNATURE----- --AqCDj3hiknadvR6t--