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.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 >>> >> 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!