Re: [PATCH v10 01/12] parallels: Out of image offset in BAT leads to image inflation

[Denis V. Lunev]

On 2/14/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
> On 03.02.23 12:18, 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). Set data_end
> > to the end of the cluster with the last correct offset.
> >
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > Reviewed-by: Denis V. Lunev <den@openvz.org>
> > ---
> >   block/parallels.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/block/parallels.c b/block/parallels.c
> > index bbea2f2221..4af68adc61 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;
> > @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, 
> > QDict *options, int flags,
> >           return ret;
> >       }
> >   +    file_size = bdrv_getlength(bs->file->bs);
> > +    if (file_size < 0) {
> > +        return -EINVAL;
> > +    }
> > +    file_size >>= BDRV_SECTOR_BITS;
>
> if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not 
> correct, DIV_ROUND_UP would be better
I would say that if file length is not aligned to block size - this is a
point to mark such file as broken and call check immediately.


> > +
> >       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
> >       if (ret < 0) {
> >           goto fail;
> > @@ -805,6 +812,16 @@ static int parallels_open(BlockDriverState *bs, 
> > QDict *options, int flags,
> >         for (i = 0; i < s->bat_size; i++) {
> >           int64_t off = bat2sect(s, i);
> > +        if (off >= file_size) {
> > +            if (flags & BDRV_O_CHECK) {
> > +                continue;
> > +            }
> > +            error_setg(errp, "parallels: Offset %" PRIi64 " in 
> > BAT[%d] entry "
> > +                       "is larger than file size (%" PRIi64 ")",
> > +                       off, i, file_size);
>
> offsets in sectors rather than in bytes may be a bit misleading

agree. Should be easy

> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> >           if (off >= s->data_end) {
> >               s->data_end = off + s->tracks;
> >           }