Re: [Qemu-block] [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 <address@hidden>
> 
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> 
> > ---
> > 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