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

[Stefan Hajnoczi]

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


signature.asc
Description: PGP signature

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 
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 
> > ---
> > 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-block] [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 


Reviewed-by: Philippe Mathieu-Daudé 


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