[v2] qcow2-bitmap: Fix uint64_t left-shift overflow

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

Posted by Tuguoyi 4 years, 6 months ago

In check_constraints_on_bitmap(), 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. As a result, for a qcow2 disk with cluster_size set to
2M, we could not even create a dirty bitmap with default
granularity. This patch fix the issue by dividing @len by
granularity instead.

Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
---
 block/qcow2-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7..71ac822 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
     }
 
     if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
-               granularity_bits))
+        (DIV_ROUND_UP(len, granularity) > (uint64_t)BME_MAX_TABLE_SIZE *
+                s->cluster_size))
     {
         error_setg(errp, "Too much space will be occupied by the bitmap. "
                    "Use larger granularity");
-- 
2.7.4

Re: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow

Posted by Max Reitz 4 years, 6 months ago

On 28.10.19 07:33, Tuguoyi wrote:
> In check_constraints_on_bitmap(), 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. As a result, for a qcow2 disk with cluster_size set to
> 2M, we could not even create a dirty bitmap with default
> granularity. This patch fix the issue by dividing @len by
> granularity instead.
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
> ---
>  block/qcow2-bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 98294a7..71ac822 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
>      }
>  
>      if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> -        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> -               granularity_bits))
> +        (DIV_ROUND_UP(len, granularity) > (uint64_t)BME_MAX_TABLE_SIZE *
> +                s->cluster_size))

This didn’t change because of this patch, but doesn’t this comparison
need a conversion of bits to bytes somewhere?

len / granularity gives us the number of bits needed for the bitmap.

BME_MAX_TABLE_SIZE is, as far as I can see, a number of bitmap clusters,
so multiplying it by the cluster size gives the number of bytes in the
bitmap.  But the number of bits is eight times higher.

Another topic: Isn’t BME_MAX_TABLE_SIZE too big?  As it is, bitmap
tables can have a size of 1 GB, and that’s the table alone.  Depending
on the cluster size, the bitmap would take up at least 64 GB and cover
at least 32 TB (at a granularity of 512 bytes).

Max

>      {
>          error_setg(errp, "Too much space will be occupied by the bitmap. "
>                     "Use larger granularity");
>

Re: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow

Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago

29.10.2019 14:14, Max Reitz wrote:
> On 28.10.19 07:33, Tuguoyi wrote:
>> In check_constraints_on_bitmap(), 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. As a result, for a qcow2 disk with cluster_size set to
>> 2M, we could not even create a dirty bitmap with default
>> granularity. This patch fix the issue by dividing @len by
>> granularity instead.
>>
>> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
>> ---
>>   block/qcow2-bitmap.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 98294a7..71ac822 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
>>       }
>>   
>>       if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
>> -        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
>> -               granularity_bits))
>> +        (DIV_ROUND_UP(len, granularity) > (uint64_t)BME_MAX_TABLE_SIZE *
>> +                s->cluster_size))
> 
> This didn’t change because of this patch, but doesn’t this comparison
> need a conversion of bits to bytes somewhere?
> 
> len / granularity gives us the number of bits needed for the bitmap.
> 
> BME_MAX_TABLE_SIZE is, as far as I can see, a number of bitmap clusters,
> so multiplying it by the cluster size gives the number of bytes in the
> bitmap.  But the number of bits is eight times higher.

I think you are right. It would be better to fix it in the same patch..

> 
> Another topic: Isn’t BME_MAX_TABLE_SIZE too big?

Maybe) Still, I don't sure that we need to change it..

>  As it is, bitmap
> tables can have a size of 1 GB, and that’s the table alone.  Depending
> on the cluster size, the bitmap would take up at least 64 GB and cover
> at least 32 TB (at a granularity of 512 bytes).
> 
> Max
> 
>>       {
>>           error_setg(errp, "Too much space will be occupied by the bitmap. "
>>                      "Use larger granularity");
>>
> 
> 


-- 
Best regards,
Vladimir

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

Posted by Tuguoyi 4 years, 6 months ago

On 29.10.2019 19:57 Vladimir Sementsov-Ogievskiy wrote:
> 29.10.2019 14:14, Max Reitz wrote:
> > On 28.10.19 07:33, Tuguoyi wrote:
> >> In check_constraints_on_bitmap(), 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. As a result, for a qcow2
> >> disk with cluster_size set to 2M, we could not even create a dirty
> >> bitmap with default granularity. This patch fix the issue by dividing
> >> @len by granularity instead.
> >>
> >> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
> >> ---
> >>   block/qcow2-bitmap.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> >> 98294a7..71ac822 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -172,8 +172,8 @@ static int
> check_constraints_on_bitmap(BlockDriverState *bs,
> >>       }
> >>
> >>       if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||

Here is the same problem, we need also to calculate the number of bits instead of bytes.

> >> -        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> >> -               granularity_bits))
> >> +        (DIV_ROUND_UP(len, granularity) >
> (uint64_t)BME_MAX_TABLE_SIZE *
> >> +                s->cluster_size))
> >
> > This didn’t change because of this patch, but doesn’t this comparison
> > need a conversion of bits to bytes somewhere?
> >
> > len / granularity gives us the number of bits needed for the bitmap.
> >
> > BME_MAX_TABLE_SIZE is, as far as I can see, a number of bitmap
> > clusters, so multiplying it by the cluster size gives the number of
> > bytes in the bitmap.  But the number of bits is eight times higher.
> 
> I think you are right. It would be better to fix it in the same patch..
> 

I think it would be better to add a inline function to calculate the max bytes 
of bitmap with given virtual size and granularity.

I'll send another patch later

> >
> > Another topic: Isn’t BME_MAX_TABLE_SIZE too big?
> 
> Maybe) Still, I don't sure that we need to change it..
> 
> >  As it is, bitmap
> > tables can have a size of 1 GB, and that’s the table alone.  Depending
> > on the cluster size, the bitmap would take up at least 64 GB and cover
> > at least 32 TB (at a granularity of 512 bytes).
> >
> > Max
> >
> >>       {
> >>           error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> >>                      "Use larger granularity");
> >>
> >
> >
> 
> 
> --
> Best regards,
> Vladimir

Re: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow

Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago

28.10.2019 9:33, Tuguoyi wrote:
> In check_constraints_on_bitmap(), 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. As a result, for a qcow2 disk with cluster_size set to
> 2M, we could not even create a dirty bitmap with default
> granularity. This patch fix the issue by dividing @len by
> granularity instead.
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>

The buggy code was introduced in 5f72826e7fc6216 (by me, sorry :(,
so it's an old bug, appeared in 2.10. Still, I add stable to CC,
as fix is very simple.

Fixes: 5f72826e7fc62167cf3a
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/qcow2-bitmap.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 98294a7..71ac822 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
>       }
>   
>       if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> -        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> -               granularity_bits))
> +        (DIV_ROUND_UP(len, granularity) > (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