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

[Denis V. Lunev]

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.

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