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

[Benoît Canet]

The Monday 12 May 2014 à 18:43:33 (+0200), Kevin Wolf wrote :
> 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.

I am under the impression that this test compute the maximum size left for
the header.

So as there is probably more that one L2 table the space left for the header
is 1 - nb_l2_table * number_of_byte_covered_by_l2 - number of byte of l1 - 
number of 
bytes of l2 themselve.

> 
> 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));
>