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

[Denis V. Lunev]

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"?
That is a different check. We need to be sure that file_size is authentic,
which worth additional check.

At my opinion, this worth additional patch later on. Let us agree here,
queue and proceed with further pending fixes.

We should use something like the following

    data_start = le32_to_cpu(s->header->data_off);
    if (data_start == 0) {
        data_start = ROUND_UP(bat_entry_off(s->bat_size), 
BDRV_SECTOR_SIZE);
    }
    if ((file_size - data_start) % cluster_size != 0) {
        error();
    }

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