* [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
@ 2019-11-01 7:37 Tuguoyi
2019-11-01 9:21 ` Vladimir Sementsov-Ogievskiy
2019-11-05 16:24 ` Max Reitz
0 siblings, 2 replies; 6+ messages in thread
From: Tuguoyi @ 2019-11-01 7:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, kwolf, mreitz, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
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 <tu.guoyi@h3c.com>
---
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");
--
2.7.4
[Patch v3]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07989.html
[Patch v2]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07490.html
[Patch v1]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07336.html
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
2019-11-01 7:37 [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow Tuguoyi
@ 2019-11-01 9:21 ` Vladimir Sementsov-Ogievskiy
2019-11-01 9:34 ` Tuguoyi
2019-11-05 16:24 ` Max Reitz
1 sibling, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-01 9:21 UTC (permalink / raw)
To: Tuguoyi, kwolf, mreitz, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
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 <tu.guoyi@h3c.com>
You forget my
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(I don't see changes except add "Fixes: " to commit msg and put get_bitmap_bytes_needed
definition header into one line.)
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
2019-11-01 9:21 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-01 9:34 ` Tuguoyi
2019-11-01 10:09 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 6+ messages in thread
From: Tuguoyi @ 2019-11-01 9:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, kwolf, mreitz, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
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 <tu.guoyi@h3c.com>
>
> You forget my
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
2019-11-01 9:34 ` Tuguoyi
@ 2019-11-01 10:09 ` Vladimir Sementsov-Ogievskiy
2019-11-02 2:27 ` Tuguoyi
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-01 10:09 UTC (permalink / raw)
To: Tuguoyi, kwolf, mreitz, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
01.11.2019 12:34, Tuguoyi wrote:
> 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 <tu.guoyi@h3c.com>
>>
>> You forget my
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Sorry for that, it's my first time to submit patch to qemu, and should I send another patch or not ?
Good start!
No, you shouldn't. Maintainer will take such marks (and may be other proposed
improvements for commit message like this "Fixes: ", when applying, so no reason to resend.
Also, when sending a new version of patch, summarize what was changed since previous version
(you may do it under "---" which follows commit message, or in cover letter if it's
a patch set with several patches.
>
>> (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
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
2019-11-01 10:09 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-02 2:27 ` Tuguoyi
0 siblings, 0 replies; 6+ messages in thread
From: Tuguoyi @ 2019-11-02 2:27 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, kwolf, mreitz, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
On 01.11.2019 18:09, Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 12:34, Tuguoyi wrote:
> > 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 <tu.guoyi@h3c.com>
> >>
> >> You forget my
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >
> > Sorry for that, it's my first time to submit patch to qemu, and should I send
> another patch or not ?
>
> Good start!
>
> No, you shouldn't. Maintainer will take such marks (and may be other
> proposed improvements for commit message like this "Fixes: ", when applying,
> so no reason to resend.
>
> Also, when sending a new version of patch, summarize what was changed
> since previous version (you may do it under "---" which follows commit
> message, or in cover letter if it's a patch set with several patches.
OK, I got it, thanks a lot.
> >
> >> (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
>
>
> --
> Best regards,
> Vladimir
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
2019-11-01 7:37 [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow Tuguoyi
2019-11-01 9:21 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-05 16:24 ` Max Reitz
1 sibling, 0 replies; 6+ messages in thread
From: Max Reitz @ 2019-11-05 16:24 UTC (permalink / raw)
To: Tuguoyi, Vladimir Sementsov-Ogievskiy, kwolf, qemu-block
Cc: Chengchiwen, qemu-devel, qemu-stable, Wangyongqing, Changlimin,
Gaoliang, Wangyong
[-- Attachment #1.1: Type: text/plain, Size: 839 bytes --]
On 01.11.19 08: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 <tu.guoyi@h3c.com>
> ---
> block/qcow2-bitmap.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
Thanks, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-05 16:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-01 7:37 [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow Tuguoyi
2019-11-01 9:21 ` Vladimir Sementsov-Ogievskiy
2019-11-01 9:34 ` Tuguoyi
2019-11-01 10:09 ` Vladimir Sementsov-Ogievskiy
2019-11-02 2:27 ` Tuguoyi
2019-11-05 16:24 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).