Re: [Qemu-devel] [PATCH 4/5] qcow1: Validate image size (CVE-2014-0223)

[Kevin Wolf]

Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:
> The Monday 12 May 2014 à 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.
> > 
> > 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).
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  block/qcow.c               | 16 ++++++++++++++--
> >  tests/qemu-iotests/092     |  9 +++++++++
> >  tests/qemu-iotests/092.out |  7 +++++++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > 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,
> >  
> >      /* read the level 1 table */
> >      shift = s->cluster_bits + s->l2_bits;
> > -    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > +    if (header.size > UINT64_MAX - (1LL << shift)) {
> 
> I won't be much helpfull but this feel wrong.
> Does each l1 entry point to an l2 chunk mapping itself to 1 << 
> (s->cluster_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 = -EINVAL;
> > +        goto fail;
> > +    } else {
> > +        uint64_t l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > +        if (l1_size > INT_MAX / sizeof(uint64_t)) {
> > +            error_setg(errp, "Image too large");
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +        s->l1_size = l1_size;
> > +    }
> >  
> >      s->l1_table_offset = header.l1_table_offset;
> >      s->l1_table = g_malloc(s->l1_size * sizeof(uint64_t));