| Message ID | 1490965980-513-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
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; >
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
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
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
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
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
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!
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
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
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
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
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;
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(-) -- 2.7.4