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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

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.
>
>> +
>>       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>           /* Image was not closed correctly. The check is mandatory */
>>           s->header_unclean = true;
>
>