Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation

["Denis V. Lunev" <den@virtuozzo.com>]

On 23.08.2022 09:23, Alexander Ivanov wrote:
>
> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>> data_end field in BDRVParallelsState is set to the biggest offset 
>>> present
>>> in BAT. If this offset is outside of the image, any further write 
>>> will create
>>> the cluster at this offset and/or the image will be truncated to this
>>> offset on close. This is definitely not correct.
>>> Raise an error in parallels_open() if data_end points outside the 
>>> image and
>>> it is not a check (let the check to repaire the image).
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>>   block/parallels.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index a229c06f25..c245ca35cd 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, 
>>> QDict *options, int flags,
>>>       BDRVParallelsState *s = bs->opaque;
>>>       ParallelsHeader ph;
>>>       int ret, size, i;
>>> +    int64_t file_size;
>>>       QemuOpts *opts = NULL;
>>>       Error *local_err = NULL;
>>>       char *buf;
>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, 
>>> QDict *options, int flags,
>>>           }
>>>       }
>>>   +    file_size = bdrv_getlength(bs->file->bs);
>>> +    if (file_size < 0) {
>>> +        ret = file_size;
>>> +        goto fail;
>>> +    }
>>> +
>>> +    file_size >>= BDRV_SECTOR_BITS;
>>> +    if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>> +        error_setg(errp, "parallels: Offset in BAT is out of image");
>>> +        ret = -EINVAL;
>>> +        goto fail;
>>> +    }
>>
>> If image is unaligned to sector size, and image size is less than 
>> s->data_end, but the difference itself is less than sector, the error 
>> message would be misleading.
>>
>> Should we consider "file_size = DIV_ROUND_UP(file_size, 
>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>
>> It's hardly possible to get such image on valid scenarios with Qemu 
>> (keeping in mind bdrv_truncate() call in parallels_close()). But it 
>> still may be possible to have such images produced by another 
>> software or by some failure path.
>>
> I think you are right, it would be better to align image size up to 
> sector size.

I would say that we need to align not on sector size but on cluster size.
That would worth additional check.