From patchwork Wed Jul 13 12:50:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 647905 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rqK302whtz9srZ for ; Wed, 13 Jul 2016 23:11:08 +1000 (AEST) Received: from localhost ([::1]:47345 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNJwQ-00064b-1E for incoming@patchwork.ozlabs.org; Wed, 13 Jul 2016 09:11:06 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNJdK-00085w-HL for qemu-devel@nongnu.org; Wed, 13 Jul 2016 08:51:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNJdI-0004GK-Ce for qemu-devel@nongnu.org; Wed, 13 Jul 2016 08:51:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34185) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNJdE-0004EQ-NI; Wed, 13 Jul 2016 08:51:16 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 41F6D883A0; Wed, 13 Jul 2016 12:51:16 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-70.ams2.redhat.com [10.36.116.70]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u6DCohED002759; Wed, 13 Jul 2016 08:51:15 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Wed, 13 Jul 2016 14:50:33 +0200 Message-Id: <1468414241-13119-27-git-send-email-kwolf@redhat.com> In-Reply-To: <1468414241-13119-1-git-send-email-kwolf@redhat.com> References: <1468414241-13119-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 13 Jul 2016 12:51:16 +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] [PULL v2 26/34] 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: kwolf@redhat.com, qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" From: Max Reitz 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 Message-id: 20160620142623.24471-3-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- 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 00c16dc..f941835 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -483,8 +483,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); @@ -500,7 +500,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; @@ -537,8 +536,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) { @@ -585,13 +587,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;