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

[Denis V. Lunev]

On 23.08.2022 11:47, Vladimir Sementsov-Ogievskiy wrote:
> On 8/23/22 12:34, Denis V. Lunev 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"?
> > 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();
> >      }
>
> Do you think that we should error-out in such conditions? It doesn't 
> break the spec, is it? Can the last cluster be half allocated?
absolutely no!

We should adjust the spec on this.

Den