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

[Alexander Ivanov]

On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote:
> On 8/17/22 22:27, Denis V. Lunev wrote:
> > On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
> > > On 8/15/22 12:02, 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 and should be fixed.
> > > >
> > > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > > > ---
> > > > v2: No change.
> > > > v3: Fix commit message.
> > > >
> > > >   block/parallels.c | 17 +++++++++++++++++
> > > >   1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/block/parallels.c b/block/parallels.c
> > > > index a229c06f25..a76cf9d993 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,22 @@ static int parallels_open(BlockDriverState 
> > > > *bs, QDict *options, int flags,
> > > >           }
> > > >       }
> > > >   +    file_size = bdrv_getlength(bs->file->bs);
> > > > +    if (file_size < 0) {
> > > > +        goto fail;
> > > > +    }
> > > > +
> > > > +    file_size >>= BDRV_SECTOR_BITS;
> > > > +    if (s->data_end > file_size) {
> > > > +        if (flags & BDRV_O_CHECK) {
> > > > +            s->data_end = file_size;
> > >
> > > Hm. but with this, any further allocation may lead to twice-allocted 
> > > clusters, as you just modify s->data_end, but havn't yet fixed the 
> > > BAT entry.. It seems unsafe. Or what I miss?
> > if O_CHECK is specified, we are going to execute parallels_co_check 
> > which
> > will correctly handle this. In the other case (normal open) we will
> > face the error, which is pretty much correct under this logic.
>
> Sounds like "s->data_end = file_size" is part of this handling and 
> should be in parallels_co_check()..
>
> Looking at it, seems more correct to recalculate s->data_end exactly 
> after for-loop, which fixes out-of-image clusters. Also it would work 
> better in case when we have leaked clusters at the end of file.
>
> Otherwise, ideally, you should have comment at top of 
> parallels_co_check(), that we must first fix out-of-image clusters, 
> before doing any kind of allocation, because data_end is already tweaked.
>
> I agree that patch should work as is.

I would like to leave this check in parallels_open(). I think it's a 
good idea to have an error on a corrupted image. Later we can replace it 
by checking&fixing images in parallels_open().

But I agree, it would be better to move the fix of s->data_end to 
parallels_co_check() and then move to parallels_check_outside_image().

> > > > +        } else {
> > > > +            error_setg(errp, "parallels: Offset in BAT is out of 
> > > > image");
> > > > +            ret = -EINVAL;
> > > > +            goto fail;
> > > > +        }
> > > > +    }
> > > > +
> > > >       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> > > >           /* Image was not closed correctly. The check is mandatory */
> > > >           s->header_unclean = true;