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

[Kevin Wolf]

Am 12.05.2014 um 19:04 hat Benoît Canet geschrieben:
> 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.

No, it doesn't. It only ensures that (header.size + (1LL << shift) - 1)
doesn't overflow, which is part of rounding up the image size.

Kevin

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