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

[Vladimir Sementsov-Ogievskiy]

On 8/18/22 11:49, Alexander Ivanov wrote:
> 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().

Yes, right, agree.

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


--
Best regards,
Vladimir