Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Denis V. Lunev]

On 03/31/2017 04:47 PM, Max Reitz wrote:
> On 31.03.2017 15:13, Peter Maydell wrote:
>> Coverity (CID 1307776) points out that in the multiply:
>>   space = to_allocate * s->tracks;
>> we are trying to calculate a 64 bit result but the types
>> of to_allocate and s->tracks mean that we actually calculate
>> a 32 bit result. Add an explicit cast to force a 64 bit
>> multiply.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> NB: compile-and-make-check tested only...
>> ---
>>  block/parallels.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 4173b3f..3886c30 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
>> int64_t sector_num,
>>  }
>>  
>>  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
>> -space = to_allocate * s->tracks;
>> +space = (int64_t)to_allocate * s->tracks;
>>  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
>> BDRV_SECTOR_BITS) {
>>  int ret;
>>  space += s->prealloc_size;
> I think the division is technically fine because to_allocate will
> roughly be *pnum / s->tracks (and since *pnum is an int, the
> multiplication cannot overflow).
>
> However, it's still good to fix this, but I would do it differently:
> Make idx, to_allocate, and i all uint64_t or int64_t instead of
> uint32_t. This would also prevent accidental overflow when storing the
> result of the division in:
>
> idx = sector_num / s->tracks;
> if (idx >= s->bat_size) {
> [...]
>
> The much greater problem to me appears to be that we don't check that
> idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
> buffer overflow in the for loop below, but I'm not sure I really want to
> know either... I think the block_status() call limits *pnum so that
> there will not be an overflow, but then we should at least assert this.
>
> Max
>
technically we are protected by the check in

static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
int64_t align, QEMUIOVector *qiov, int flags)
...
/* Forward the request to the BlockDriver, possibly fragmenting it */
total_bytes = bdrv_getlength(bs);
if (total_bytes < 0) {
ret = total_bytes;
goto out;
}

max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
if (bytes <= max_bytes && bytes <= max_transfer) {
ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
goto out;
}

which guarantees that the request is always inside the length of the
device. Thus we should be on the safe side with the mentioned
access as bat_size is calculated from the size of the entire virtual
disk.

Den

Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Max Reitz]

On 31.03.2017 18:20, Stefan Hajnoczi wrote:
> On Fri, Mar 31, 2017 at 03:47:39PM +0200, Max Reitz wrote:
>> On 31.03.2017 15:13, Peter Maydell wrote:
>>> Coverity (CID 1307776) points out that in the multiply:
>>>   space = to_allocate * s->tracks;
>>> we are trying to calculate a 64 bit result but the types
>>> of to_allocate and s->tracks mean that we actually calculate
>>> a 32 bit result. Add an explicit cast to force a 64 bit
>>> multiply.
>>>
>>> Signed-off-by: Peter Maydell 
>>> ---
>>> NB: compile-and-make-check tested only...
>>> ---
>>>  block/parallels.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index 4173b3f..3886c30 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
>>> int64_t sector_num,
>>>  }
>>>  
>>>  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
>>> -space = to_allocate * s->tracks;
>>> +space = (int64_t)to_allocate * s->tracks;
>>>  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
>>> BDRV_SECTOR_BITS) {
>>>  int ret;
>>>  space += s->prealloc_size;
>>
>> I think the division is technically fine because to_allocate will
>> roughly be *pnum / s->tracks (and since *pnum is an int, the
>> multiplication cannot overflow).
>>
>> However, it's still good to fix this, but I would do it differently:
>> Make idx, to_allocate, and i all uint64_t or int64_t instead of
>> uint32_t. This would also prevent accidental overflow when storing the
>> result of the division in:
>>
>> idx = sector_num / s->tracks;
>> if (idx >= s->bat_size) {
>> [...]
>>
>> The much greater problem to me appears to be that we don't check that
>> idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
>> buffer overflow in the for loop below, but I'm not sure I really want to
>> know either... I think the block_status() call limits *pnum so that
>> there will not be an overflow, but then we should at least assert this.
> 
> Will you send a new patch that supercedes this one?

Well, since you're asking so nicely...

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Stefan Hajnoczi]

On Fri, Mar 31, 2017 at 03:47:39PM +0200, Max Reitz wrote:
> On 31.03.2017 15:13, Peter Maydell wrote:
> > Coverity (CID 1307776) points out that in the multiply:
> >   space = to_allocate * s->tracks;
> > we are trying to calculate a 64 bit result but the types
> > of to_allocate and s->tracks mean that we actually calculate
> > a 32 bit result. Add an explicit cast to force a 64 bit
> > multiply.
> > 
> > Signed-off-by: Peter Maydell 
> > ---
> > NB: compile-and-make-check tested only...
> > ---
> >  block/parallels.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/parallels.c b/block/parallels.c
> > index 4173b3f..3886c30 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
> > int64_t sector_num,
> >  }
> >  
> >  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
> > -space = to_allocate * s->tracks;
> > +space = (int64_t)to_allocate * s->tracks;
> >  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
> > BDRV_SECTOR_BITS) {
> >  int ret;
> >  space += s->prealloc_size;
> 
> I think the division is technically fine because to_allocate will
> roughly be *pnum / s->tracks (and since *pnum is an int, the
> multiplication cannot overflow).
> 
> However, it's still good to fix this, but I would do it differently:
> Make idx, to_allocate, and i all uint64_t or int64_t instead of
> uint32_t. This would also prevent accidental overflow when storing the
> result of the division in:
> 
> idx = sector_num / s->tracks;
> if (idx >= s->bat_size) {
> [...]
> 
> The much greater problem to me appears to be that we don't check that
> idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
> buffer overflow in the for loop below, but I'm not sure I really want to
> know either... I think the block_status() call limits *pnum so that
> there will not be an overflow, but then we should at least assert this.

Will you send a new patch that supercedes this one?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Max Reitz]

On 31.03.2017 16:54, Denis V. Lunev wrote:
> On 03/31/2017 04:47 PM, Max Reitz wrote:
>> On 31.03.2017 15:13, Peter Maydell wrote:
>>> Coverity (CID 1307776) points out that in the multiply:
>>>   space = to_allocate * s->tracks;
>>> we are trying to calculate a 64 bit result but the types
>>> of to_allocate and s->tracks mean that we actually calculate
>>> a 32 bit result. Add an explicit cast to force a 64 bit
>>> multiply.
>>>
>>> Signed-off-by: Peter Maydell 
>>> ---
>>> NB: compile-and-make-check tested only...
>>> ---
>>>  block/parallels.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index 4173b3f..3886c30 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
>>> int64_t sector_num,
>>>  }
>>>  
>>>  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
>>> -space = to_allocate * s->tracks;
>>> +space = (int64_t)to_allocate * s->tracks;
>>>  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
>>> BDRV_SECTOR_BITS) {
>>>  int ret;
>>>  space += s->prealloc_size;
>> I think the division is technically fine because to_allocate will
>> roughly be *pnum / s->tracks (and since *pnum is an int, the
>> multiplication cannot overflow).
>>
>> However, it's still good to fix this, but I would do it differently:
>> Make idx, to_allocate, and i all uint64_t or int64_t instead of
>> uint32_t. This would also prevent accidental overflow when storing the
>> result of the division in:
>>
>> idx = sector_num / s->tracks;
>> if (idx >= s->bat_size) {
>> [...]
>>
>> The much greater problem to me appears to be that we don't check that
>> idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
>> buffer overflow in the for loop below, but I'm not sure I really want to
>> know either... I think the block_status() call limits *pnum so that
>> there will not be an overflow, but then we should at least assert this.
>>
>> Max
>>
> technically we are protected by the check in
> 
> static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
> BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
> int64_t align, QEMUIOVector *qiov, int flags)
> ...
> /* Forward the request to the BlockDriver, possibly fragmenting it */
> total_bytes = bdrv_getlength(bs);
> if (total_bytes < 0) {
> ret = total_bytes;
> goto out;
> }
> 
> max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
> if (bytes <= max_bytes && bytes <= max_transfer) {
> ret = bdrv_driver_preadv(bs, offset, bytes, qiov, 0);
> goto out;
> }
> 
> which guarantees that the request is always inside the length of the
> device. Thus we should be on the safe side with the mentioned
> access as bat_size is calculated from the size of the entire virtual
> disk.

Right, but then we wouldn't need the check on idx. With the way things
are, it looks a bit confusing. Maybe we should just make it an assertion?

assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Max Reitz]

On 31.03.2017 15:13, Peter Maydell wrote:
> Coverity (CID 1307776) points out that in the multiply:
>   space = to_allocate * s->tracks;
> we are trying to calculate a 64 bit result but the types
> of to_allocate and s->tracks mean that we actually calculate
> a 32 bit result. Add an explicit cast to force a 64 bit
> multiply.
> 
> Signed-off-by: Peter Maydell 
> ---
> NB: compile-and-make-check tested only...
> ---
>  block/parallels.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 4173b3f..3886c30 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
> int64_t sector_num,
>  }
>  
>  to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
> -space = to_allocate * s->tracks;
> +space = (int64_t)to_allocate * s->tracks;
>  if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
> BDRV_SECTOR_BITS) {
>  int ret;
>  space += s->prealloc_size;

I think the division is technically fine because to_allocate will
roughly be *pnum / s->tracks (and since *pnum is an int, the
multiplication cannot overflow).

However, it's still good to fix this, but I would do it differently:
Make idx, to_allocate, and i all uint64_t or int64_t instead of
uint32_t. This would also prevent accidental overflow when storing the
result of the division in:

idx = sector_num / s->tracks;
if (idx >= s->bat_size) {
[...]

The much greater problem to me appears to be that we don't check that
idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
buffer overflow in the for loop below, but I'm not sure I really want to
know either... I think the block_status() call limits *pnum so that
there will not be an overflow, but then we should at least assert this.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Peter Maydell]

Coverity (CID 1307776) points out that in the multiply:
  space = to_allocate * s->tracks;
we are trying to calculate a 64 bit result but the types
of to_allocate and s->tracks mean that we actually calculate
a 32 bit result. Add an explicit cast to force a 64 bit
multiply.

Signed-off-by: Peter Maydell 
---
NB: compile-and-make-check tested only...
---
 block/parallels.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 4173b3f..3886c30 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
 }
 
 to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
-space = to_allocate * s->tracks;
+space = (int64_t)to_allocate * s->tracks;
 if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
BDRV_SECTOR_BITS) {
 int ret;
 space += s->prealloc_size;
-- 
2.7.4