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.mayd...@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
signature.asc
Description: OpenPGP digital signature