From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkwPx-0006cb-Kz for qemu-devel@nongnu.org; Thu, 15 May 2014 10:13:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkwPq-0000YX-GR for qemu-devel@nongnu.org; Thu, 15 May 2014 10:13:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9602) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkwPq-0000YP-9Y for qemu-devel@nongnu.org; Thu, 15 May 2014 10:13:46 -0400 Date: Thu, 15 May 2014 16:13:42 +0200 From: Kevin Wolf Message-ID: <20140515141342.GH3822@noname.redhat.com> References: <1399899851-5641-1-git-send-email-kwolf@redhat.com> <1399899851-5641-3-git-send-email-kwolf@redhat.com> <20140512150026.GE7858@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140512150026.GE7858@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/5] qcow1: Check maximum cluster size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: qemu-devel@nongnu.org, stefanha@redhat.com, ppandit@redhat.com Am 12.05.2014 um 17:00 hat Beno=EEt Canet geschrieben: > The Monday 12 May 2014 =E0 15:04:08 (+0200), Kevin Wolf wrote : > > Huge values for header.cluster_bits cause unbounded allocations (e.g. > > for s->cluster_cache) and crash qemu this way. Less huge values may > > survive those allocations, but can cause integer overflows later on. > >=20 > > The only cluster sizes that qemu can create are 4k (for standalone > > images) and 512 (for images with backing files), so we can limit it > > to 64k. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block/qcow.c | 10 ++++++-- > > tests/qemu-iotests/092 | 60 ++++++++++++++++++++++++++++++++++++= ++++++++++ > > tests/qemu-iotests/092.out | 9 +++++++ > > tests/qemu-iotests/group | 1 + > > 4 files changed, 78 insertions(+), 2 deletions(-) > > create mode 100755 tests/qemu-iotests/092 > > create mode 100644 tests/qemu-iotests/092.out > >=20 > > diff --git a/block/qcow.c b/block/qcow.c > > index 3684794..e60df23 100644 > > --- a/block/qcow.c > > +++ b/block/qcow.c > > @@ -128,11 +128,17 @@ static int qcow_open(BlockDriverState *bs, QDic= t *options, int flags, > > goto fail; > > } > > =20 > > - if (header.size <=3D 1 || header.cluster_bits < 9) { > > - error_setg(errp, "invalid value in qcow header"); > > + if (header.size <=3D 1) { > > + error_setg(errp, "Image size is too small (must be at least = 2 bytes)"); > > ret =3D -EINVAL; > > goto fail; > > } > > + if (header.cluster_bits < 9 || header.cluster_bits > 16) { > > + error_setg(errp, "Cluster size must be between 512 and 64k")= ; > > + ret =3D -EINVAL; > > + goto fail; > > + } > > + > > if (header.crypt_method > QCOW_CRYPT_AES) { > > error_setg(errp, "invalid encryption method in qcow header")= ; > > ret =3D -EINVAL; > > diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092 > > new file mode 100755 > > index 0000000..b0f04e3 > > --- /dev/null > > +++ b/tests/qemu-iotests/092 > > @@ -0,0 +1,60 @@ > > +#!/bin/bash > > +# > > +# qcow1 format input validation tests > > +# > > +# Copyright (C) 2014 Red Hat, Inc. > > +# > > +# This program is free software; you can redistribute it and/or modi= fy > > +# it under the terms of the GNU General Public License as published = by > > +# the Free Software Foundation; either version 2 of the License, or > > +# (at your option) any later version. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program. If not, see . > > +# > > + > > +# creator > > +owner=3Dkwolf@redhat.com > > + > > +seq=3D`basename $0` > > +echo "QA output created by $seq" > > + > > +here=3D`pwd` > > +tmp=3D/tmp/$$ > > +status=3D1 # failure is the default! > > + > > +_cleanup() > > +{ > > + rm -f $TEST_IMG.snap > > + _cleanup_test_img > > +} > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > + > > +# get standard environment, filters and checks > > +. ./common.rc > > +. ./common.filter > > + > > +_supported_fmt qcow > > +_supported_proto generic > > +_supported_os Linux > > + > > +offset_cluster_bits=3D32 >=20 > > +offset_l2_bits=3D33 > This seems to be an extra. Moving this line to the next patch. > > + > > +echo > > +echo "=3D=3D Invalid cluster size =3D=3D" > > +_make_test_img 64M >=20 >=20 > > +poke_file "$TEST_IMG" "$offset_cluster_bits" "\xff" > > +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _fi= lter_testdir > > +poke_file "$TEST_IMG" "$offset_cluster_bits" "\x1f" > > +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _fi= lter_testdir >=20 > From the code " + if (header.cluster_bits < 9 || header.cluster_bits= > 16) {" >=20 > Shouldn't the hex values be "\x08" and "\x11" ? Those values actually kind of work, it needs something bigger to crash qemu. But I'll add those two values as well. Kevin