From patchwork Mon Jun 20 14:26:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Max Reitz X-Patchwork-Id: 9187661 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D16EB601C0 for ; Mon, 20 Jun 2016 14:42:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BF1042787D for ; Mon, 20 Jun 2016 14:42:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B3DEA2793D; Mon, 20 Jun 2016 14:42:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4BD7F2787D for ; Mon, 20 Jun 2016 14:42:27 +0000 (UTC) Received: from localhost ([::1]:44055 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bF0PC-00052S-1X for patchwork-qemu-devel@patchwork.kernel.org; Mon, 20 Jun 2016 10:42:26 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bF0A3-0005aV-ID for qemu-devel@nongnu.org; Mon, 20 Jun 2016 10:26:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bF0A1-0002fS-D4 for qemu-devel@nongnu.org; Mon, 20 Jun 2016 10:26:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56719) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bF09n-0002dq-Dk; Mon, 20 Jun 2016 10:26:31 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B9A6A7F097; Mon, 20 Jun 2016 14:26:30 +0000 (UTC) Received: from localhost (ovpn-116-123.ams2.redhat.com [10.36.116.123]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5KEQTwU030390 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 20 Jun 2016 10:26:30 -0400 From: Max Reitz To: qemu-block@nongnu.org Date: Mon, 20 Jun 2016 16:26:23 +0200 Message-Id: <20160620142623.24471-3-mreitz@redhat.com> In-Reply-To: <20160620142623.24471-1-mreitz@redhat.com> References: <20160620142623.24471-1-mreitz@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 20 Jun 2016 14:26:30 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 2/2] qcow2: Fix qcow2_get_cluster_offset() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , John Snow , qemu-devel@nongnu.org, Max Reitz Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Recently, qcow2_get_cluster_offset() has been changed to work with bytes instead of sectors. This invalidated some assertions and introduced a possible integer multiplication overflow. This could be reproduced using e.g. $ qemu-img create -f qcow2 -o cluster_size=1M blub.qcow2 8G Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 encryption=off cluster_size=1048576 lazy_refcounts=off refcount_bits=16 $ qemu-io -c map blub.qcow2 qemu-io: qemu/block/qcow2-cluster.c:504: qcow2_get_cluster_offset: Assertion `bytes_needed <= INT_MAX' failed. [1] 20775 abort (core dumped) qemu-io -c map foo.qcow2 This patch removes the now wrong assertion, adding comments and more assertions to prove its correctness (and fixing the overflow which would become apparent with the original assertion removed). Signed-off-by: Max Reitz Reviewed-by: Eric Blake --- block/qcow2-cluster.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 893ddf6..b942fb5 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -484,8 +484,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, unsigned int l2_index; uint64_t l1_index, l2_offset, *l2_table; int l1_bits, c; - unsigned int offset_in_cluster, nb_clusters; - uint64_t bytes_available, bytes_needed; + unsigned int offset_in_cluster; + uint64_t bytes_available, bytes_needed, nb_clusters; int ret; offset_in_cluster = offset_into_cluster(s, offset); @@ -501,7 +501,6 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, if (bytes_needed > bytes_available) { bytes_needed = bytes_available; } - assert(bytes_needed <= INT_MAX); *cluster_offset = 0; @@ -538,8 +537,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1); *cluster_offset = be64_to_cpu(l2_table[l2_index]); - /* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */ nb_clusters = size_to_clusters(s, bytes_needed); + /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned + * integers; the minimum cluster size is 512, so this assertion is always + * true */ + assert(nb_clusters <= INT_MAX); ret = qcow2_get_cluster_type(*cluster_offset); switch (ret) { @@ -586,13 +588,17 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); - bytes_available = (c * s->cluster_size); + bytes_available = (int64_t)c * s->cluster_size; out: if (bytes_available > bytes_needed) { bytes_available = bytes_needed; } + /* bytes_available <= bytes_needed <= *bytes + offset_in_cluster; + * subtracting offset_in_cluster will therefore definitely yield something + * not exceeding UINT_MAX */ + assert(bytes_available - offset_in_cluster <= UINT_MAX); *bytes = bytes_available - offset_in_cluster; return ret;