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

[Vladimir Sementsov-Ogievskiy]

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.


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


--
Best regards,
Vladimir