Date: Mon, 12 May 2014 18:43:33 +0200
From: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20140512164333.GE4371@noname.redhat.com>
References: <1399899851-5641-1-git-send-email-kwolf@redhat.com>
	<1399899851-5641-5-git-send-email-kwolf@redhat.com>
	<20140512155024.GG7858@irqsave.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <20140512155024.GG7858@irqsave.net>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH 4/5] qcow1: Validate image size
 (CVE-2014-0223)
To: =?iso-8859-1?Q?Beno=EEt?= Canet <benoit.canet@irqsave.net>
Cc: qemu-devel@nongnu.org, stefanha@redhat.com, ppandit@redhat.com

Am 12.05.2014 um 17:50 hat Beno=EEt Canet geschrieben:
> The Monday 12 May 2014 =E0 15:04:10 (+0200), Kevin Wolf wrote :
> > A huge image size could cause s->l1_size to overflow. Make sure that
> > images never require a L1 table larger than what fits in s->l1_size.
> >=20
> > This cannot only cause unbounded allocations, but also the allocation=
 of
> > a too small L1 table, resulting in out-of-bounds array accesses (both
> > reads and writes).
> >=20
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow.c               | 16 ++++++++++++++--
> >  tests/qemu-iotests/092     |  9 +++++++++
> >  tests/qemu-iotests/092.out |  7 +++++++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> >=20
> > diff --git a/block/qcow.c b/block/qcow.c
> > index e8038e5..3566c05 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> > @@ -61,7 +61,7 @@ typedef struct BDRVQcowState {
> >      int cluster_sectors;
> >      int l2_bits;
> >      int l2_size;
> > -    int l1_size;
> > +    unsigned int l1_size;
> >      uint64_t cluster_offset_mask;
> >      uint64_t l1_table_offset;
> >      uint64_t *l1_table;
> > @@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDict=
 *options, int flags,
> > =20
> >      /* read the level 1 table */
> >      shift =3D s->cluster_bits + s->l2_bits;
> > -    s->l1_size =3D (header.size + (1LL << shift) - 1) >> shift;
> > +    if (header.size > UINT64_MAX - (1LL << shift)) {
>=20
> I won't be much helpfull but this feel wrong.
> Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->clus=
ter_bits + s->l2_bits) bytes ?
> Where the size for the L2 chunk themselves is accounted ?

Not sure what your concern is, but this is basically the same system as
with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map
virtual disk clusters to image file clusters. They don't map metadata
like themselves.

One cluster contains (1 << cluster_bits) bytes. One L2 table contains
mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits +
l2_bits)) is the number of bytes on the virtual disk that are described
by a single L2 table.

All of this is not related to this patch. All I'm doing here is catching
integer overflows in the calculation of s->l1_size. Apart from error
cases, the calculation is unchanged.

Kevin

> > +        error_setg(errp, "Image too large");
> > +        ret =3D -EINVAL;
> > +        goto fail;
> > +    } else {
> > +        uint64_t l1_size =3D (header.size + (1LL << shift) - 1) >> s=
hift;
> > +        if (l1_size > INT_MAX / sizeof(uint64_t)) {
> > +            error_setg(errp, "Image too large");
> > +            ret =3D -EINVAL;
> > +            goto fail;
> > +        }
> > +        s->l1_size =3D l1_size;
> > +    }
> > =20
> >      s->l1_table_offset =3D header.l1_table_offset;
> >      s->l1_table =3D g_malloc(s->l1_size * sizeof(uint64_t));