The Monday 12 May 2014 à 19:04:22 (+0200), Benoît Canet wrote :
> 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 <kw...@redhat.com>
> > > > ---
> > > > 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.
I got this part wrong but still we must account that there could be multiple l2
tables.
>
> >
> > 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));
> >
>
