Re: [PATCH v10 01/12] parallels: Out of image offset in BAT leads to image inflation

[Vladimir Sementsov-Ogievskiy]

On 03.02.23 12:18, 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). Set data_end
> to the end of the cluster with the last correct offset.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index bbea2f2221..4af68adc61 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;
> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>           return ret;
>       }
>   
> +    file_size = bdrv_getlength(bs->file->bs);
> +    if (file_size < 0) {
> +        return -EINVAL;
> +    }
> +    file_size >>= BDRV_SECTOR_BITS;

if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not correct, 
DIV_ROUND_UP would be better

> +
>       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
>       if (ret < 0) {
>           goto fail;
> @@ -805,6 +812,16 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>   
>       for (i = 0; i < s->bat_size; i++) {
>           int64_t off = bat2sect(s, i);
> +        if (off >= file_size) {
> +            if (flags & BDRV_O_CHECK) {
> +                continue;
> +            }
> +            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> +                       "is larger than file size (%" PRIi64 ")",
> +                       off, i, file_size);

offsets in sectors rather than in bytes may be a bit misleading

> +            ret = -EINVAL;
> +            goto fail;
> +        }
>           if (off >= s->data_end) {
>               s->data_end = off + s->tracks;
>           }

--
Best regards,
Vladimir