Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Denis V. Lunev]

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