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

[Qemu-devel] [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 <peter.maydell@linaro.org>
---
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

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

[Philippe Mathieu-Daudé]

Hi,

Eduardo you seem skilled regarding Coccinelle scripts, is it possible to 
write one to find those overflows?

Peter having one more macro might help or confuses more?

#define MULTIPLY64(a32, b32) ((int64_t)a32 * b32)

On 03/31/2017 10:13 AM, 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 <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> 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;
>

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

[Peter Maydell]

On 31 March 2017 at 14:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> write one to find those overflows?

This is the final one that Coverity reports on the current
codebase.

> Peter having one more macro might help or confuses more?
>
> #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32)

We've fixed them with casts generally in the past.

thanks
-- PMM

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

[Eduardo Habkost]

On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> write one to find those overflows?

Probably not. AFAIK, Coccinelle rules are based on local code
syntax only. This means it doesn't know the data type of
expressions like (s->tracks).

> 
> Peter having one more macro might help or confuses more?
> 
> #define MULTIPLY64(a32, b32) ((int64_t)a32 * b32)
> 
> On 03/31/2017 10:13 AM, 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 <peter.maydell@linaro.org>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> > ---
> > 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;
> > 

-- 
Eduardo

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Fri, Mar 31, 2017 at 10:40:33AM -0300, Eduardo Habkost wrote:
> On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote:
> > Hi,
> > 
> > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> > write one to find those overflows?
> 
> Probably not. AFAIK, Coccinelle rules are based on local code
> syntax only. This means it doesn't know the data type of
> expressions like (s->tracks).

I'm surprised by that statement.  Coccinelle isn't a text matcher, it's
a proper C compiler frontend that parses the all code in the compilation
unit.  Therefore it must have the type information even for s->tracks.

Disclaimer: This should in no way be considered a volunteer offer to
write cocci scripts now or at any time in the future :).  I'm not fluent
in the semantic patch syntax.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Eduardo Habkost]

On Fri, Mar 31, 2017 at 05:18:39PM +0100, Stefan Hajnoczi wrote:
> On Fri, Mar 31, 2017 at 10:40:33AM -0300, Eduardo Habkost wrote:
> > On Fri, Mar 31, 2017 at 10:27:44AM -0300, Philippe Mathieu-Daudé wrote:
> > > Hi,
> > > 
> > > Eduardo you seem skilled regarding Coccinelle scripts, is it possible to
> > > write one to find those overflows?
> > 
> > Probably not. AFAIK, Coccinelle rules are based on local code
> > syntax only. This means it doesn't know the data type of
> > expressions like (s->tracks).
> 
> I'm surprised by that statement.  Coccinelle isn't a text matcher, it's
> a proper C compiler frontend that parses the all code in the compilation
> unit.  Therefore it must have the type information even for s->tracks.

You are probably not wrong about it not being just a text
matcher. But I'm not sure about it being able to have type
information for s->tracks. The documentation isn't clear about
that.

The 'idexpression' declarations seems to accept some kind of C
type annotations (I didn't know that!), but the documentation
also says: "A more complex description of a location, such as
a->b is considered to be an expression, not an idexpression".
And 'expression' metavariables don't seem to support type
annotations.

My impression is that Coccinelle has limited support to
understand simple variable declarations, but not the full set of
C type declarations and type system rules that would allow it to
figure out the type of an expression like s->tracks.

But I really hope to be wrong, because that would be very useful. :)

> Disclaimer: This should in no way be considered a volunteer offer to
> write cocci scripts now or at any time in the future :).  I'm not fluent
> in the semantic patch syntax.

I don't believe there's anybody in the world fluent in the SmPL
syntax. Maybe its authors are, but I wouldn't be so sure. :)

-- 
Eduardo

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

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 <peter.maydell@linaro.org>
> ---
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [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 <peter.maydell@linaro.org>
>> ---
>> 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-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3268 bytes --]

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 <peter.maydell@linaro.org>
>>> ---
>>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

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

[Denis V. Lunev]

On 03/31/2017 05:56 PM, Max Reitz wrote:
> 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 <peter.maydell@linaro.org>
>>>> ---
>>>> 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
>
>

good idea!

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]

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 <peter.maydell@linaro.org>
> > ---
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

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 <peter.maydell@linaro.org>
>>> ---
>>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]