RE: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow

[Tuguoyi]

On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 10:37, Tuguoyi wrote:
> > There are two issues in In check_constraints_on_bitmap(),
> > 1) The sanity check on the granularity will cause uint64_t integer
> > left-shift overflow when cluster_size is 2M and the granularity is
> > BIGGER than 32K.
> > 2) The way to calculate image size that the maximum bitmap supported
> > can map to is a bit incorrect.
> > This patch fix it by add a helper function to calculate the number of
> > bytes needed by a normal bitmap in image and compare it to the maximum
> > bitmap bytes supported by qemu.
> >
> > Fixes: 5f72826e7fc62167cf3a
> > Signed-off-by: Guoyi Tu <address@hidden>
> 
> You forget my
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Sorry for that, it's my first time to submit patch to qemu, and should I send 
another patch or not ?

> (I don't see changes except add "Fixes: " to commit msg and put
> get_bitmap_bytes_needed definition header into one line.)

Yes, Only minor changes are made, including removing 'inline' keyword.

Thanks for your patience in reviewing

> > ---
> >   block/qcow2-bitmap.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> > 98294a7..ef9ef62 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int
> cluster_size)
> >       return 0;
> >   }
> >
> > +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t
> > +granularity) {
> > +    int64_t num_bits = DIV_ROUND_UP(len, granularity);
> > +
> > +    return DIV_ROUND_UP(num_bits, 8); }
> > +
> >   static int check_constraints_on_bitmap(BlockDriverState *bs,
> >                                          const char *name,
> >                                          uint32_t granularity, @@
> > -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState
> *bs,
> >       BDRVQcow2State *s = bs->opaque;
> >       int granularity_bits = ctz32(granularity);
> >       int64_t len = bdrv_getlength(bs);
> > +    int64_t bitmap_bytes;
> >
> >       assert(granularity > 0);
> >       assert((granularity & (granularity - 1)) == 0); @@ -171,9 +179,9
> > @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
> >           return -EINVAL;
> >       }
> >
> > -    if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> > -        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> > -               granularity_bits))
> > +    bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
> > +    if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
> > +        (bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE *
> > + s->cluster_size))
> >       {
> >           error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> >                      "Use larger granularity");
> >
> 
> 
> --
> Best regards,
> Vladimir