From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEHFt-0005zD-US for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:16:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEHFn-0006xC-US for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:16:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39078) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEHFn-0006x3-MQ for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:16:07 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7RBG6mV019920 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 27 Aug 2013 07:16:06 -0400 Date: Tue, 27 Aug 2013 13:16:18 +0200 From: Kevin Wolf Message-ID: <20130827111618.GC648@dhcp-200-207.str.redhat.com> References: <1377522260-16676-1-git-send-email-mreitz@redhat.com> <1377522260-16676-3-git-send-email-mreitz@redhat.com> <20130827101757.GB648@dhcp-200-207.str.redhat.com> <521C8819.5070508@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <521C8819.5070508@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/5] qcow2: Metadata overlap checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 27.08.2013 um 13:06 hat Max Reitz geschrieben: > Am 27.08.2013 12:17, schrieb Kevin Wolf: > >Am 26.08.2013 um 15:04 hat Max Reitz geschrieben: > >>Two new functions are added; the first one checks a given range in the > >>image file for overlaps with metadata (main header, L1 tables, L2 > >>tables, refcount table and blocks). > >> > >>The second one should be used immediately before writing to the image > >>file as it calls the first function and, upon collision, marks the > >>image as corrupt and makes the BDS unusable, thereby preventing > >>further access. > >> > >>Both functions take a bitmask argument specifying the structures which > >>should be checked for overlaps, making it possible to also check > >>metadata writes against colliding with other structures. > >> > >>Signed-off-by: Max Reitz > >>--- > >> block/qcow2-refcount.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> block/qcow2.h | 28 ++++++++++ > >> 2 files changed, 170 insertions(+) > >>+ int64_t offset, int64_t size) > >>+{ > >>+ BDRVQcowState *s = bs->opaque; > >>+ int i, j; > >>+ > >>+ if (!size) { > >>+ return 0; > >>+ } > >>+ > >>+ if (chk & QCOW2_OL_MAIN_HEADER) { > >>+ if (offset < s->cluster_size) { > >>+ return QCOW2_OL_MAIN_HEADER; > >>+ } > >>+ } > >>+ > >>+ if ((chk & QCOW2_OL_ACTIVE_L1) && s->l1_size) { > >>+ if (ranges_overlap(offset, size, s->l1_table_offset, > >>+ s->l1_size * sizeof(uint64_t))) { > >The size could be rounded up to the next cluster boundary (same thing > >for other metadata types). > Would this actually change anything? Not sure. With correct images, it wouldn't, because both the old and the new offset would be aligned to a cluster boundary, so if there is an overlap, it would be at the start. But after all, we're dealing with broken images here. I don't have a strong opinion on it, take it as a suggestion that you can implement for additional safety. But if you don't want to, I don't think it's a horrible gap in the checks. Kevin