| Message ID | 20220822090517.2289697-2-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Refactor the code of images checks and fix a bug | expand |
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"? It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. > + > if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { > /* Image was not closed correctly. The check is mandatory */ > s->header_unclean = true;
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"? > > It's hardly possible to get such image on valid scenarios with Qemu > (keeping in mind bdrv_truncate() call in parallels_close()). But it > still may be possible to have such images produced by another software > or by some failure path. > I think you are right, it would be better to align image size up to sector size. > >> + >> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >> /* Image was not closed correctly. The check is mandatory */ >> s->header_unclean = true; > >
On 8/23/22 10:23, Alexander Ivanov 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. Actually, it's correct, as don't break the spec at docs/interop/parallels.txt. So, it's bad but still correct.. So, we probably can restrict it in Qemu if we want. Do we? Now I doubt. We are going to refuse images with leaks even for read. That means, qemu-img convert will stop working for images with leaks, you'll have to fix the image first, which may fail. >>> 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"? >> >> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >> > I think you are right, it would be better to align image size up to sector size. Hmm, or even to cluster_size? When last cluster is unallocated. But cluster boundary is not required to be aligned to cluster size.. Anyway, now I think it's wrong to restrict opening file with leaks of any kind. That breaks qemu-img convert, and that's not how other formats behave. >> >>> + >>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>> /* Image was not closed correctly. The check is mandatory */ >>> s->header_unclean = true; >> >>
On 23.08.2022 09:23, Alexander Ivanov 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"? >> >> It's hardly possible to get such image on valid scenarios with Qemu >> (keeping in mind bdrv_truncate() call in parallels_close()). But it >> still may be possible to have such images produced by another >> software or by some failure path. >> > I think you are right, it would be better to align image size up to > sector size. I would say that we need to align not on sector size but on cluster size. That would worth additional check.
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(); } > > It's hardly possible to get such image on valid scenarios with Qemu > (keeping in mind bdrv_truncate() call in parallels_close()). But it > still may be possible to have such images produced by another software > or by some failure path. > > >> + >> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >> /* Image was not closed correctly. The check is mandatory */ >> s->header_unclean = true; > >
On 8/23/22 10:59, Vladimir Sementsov-Ogievskiy wrote: > On 8/23/22 10:23, Alexander Ivanov 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. > > Actually, it's correct, as don't break the spec at docs/interop/parallels.txt. > > So, it's bad but still correct.. So, we probably can restrict it in Qemu if we want. Do we? > > Now I doubt. We are going to refuse images with leaks even for read. That means, qemu-img convert will stop working for images with leaks, you'll have to fix the image first, which may fail. > >>>> 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"? >>> >>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >>> >> I think you are right, it would be better to align image size up to sector size. > > > Hmm, or even to cluster_size? When last cluster is unallocated. But cluster boundary is not required to be aligned to cluster size.. > > Anyway, now I think it's wrong to restrict opening file with leaks of any kind. That breaks qemu-img convert, and that's not how other formats behave. > >>> >>>> + >>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>>> /* Image was not closed correctly. The check is mandatory */ >>>> s->header_unclean = true; >>> >>> > > Oops, ignore this. We are not about leaks (when file size > data_end), we are about wrong clusters when data_end > file_size.
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? > >> >> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >> >> >>> + >>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>> /* Image was not closed correctly. The check is mandatory */ >>> s->header_unclean = true; >> >> >
On 23.08.2022 11:20, Denis V. Lunev wrote: > On 23.08.2022 09:23, Alexander Ivanov 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"? >>> >>> It's hardly possible to get such image on valid scenarios with Qemu >>> (keeping in mind bdrv_truncate() call in parallels_close()). But it >>> still may be possible to have such images produced by another >>> software or by some failure path. >>> >> I think you are right, it would be better to align image size up to >> sector size. > > I would say that we need to align not on sector size but on cluster size. > That would worth additional check. I agree on it for "WithouFreSpacExt" image format. For "WithoutFreeSpace" format there are two restrictions: 1) the result of (cluster offset - data offset) must be aligned to cluster size: 2) data_off should be aligned to sector size (0 in data_off leads to the same restriction). So file size can be aligned on sector size, but not aligned on cluster size. Should we check dependent on image format?
On 22.08.2022 11: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 (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { > /* Image was not closed correctly. The check is mandatory */ > s->header_unclean = true; Reviewed-by: Denis V. Lunev <den@openvz.org>
On 8/23/22 12:20, Denis V. Lunev wrote: > On 23.08.2022 09:23, Alexander Ivanov 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"? >>> >>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >>> >> I think you are right, it would be better to align image size up to sector size. > > I would say that we need to align not on sector size but on cluster size. > That would worth additional check. And not simply align, as data_offset is not necessarily aligned to cluster size. Finally, what should we check? I suggest diff --git a/block/parallels.c b/block/parallels.c index 6d4ed77f16..b882ea1200 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -725,6 +725,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; @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, return -EINVAL; } + file_size = bdrv_getlength(bs->file->bs); + if (file_size < 0) { + return file_size; + } + ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); if (ret < 0) { goto fail; @@ -798,6 +804,13 @@ 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) { + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " + "is larger than file size (%" PRIi64 ")", + off, i, file_size); + ret = -EINVAL; + goto fail; + } if (off >= s->data_end) { s->data_end = off + s->tracks; } - better error message, and we check exactly what's written in the spec (docs/interop/parallels.c): Cluster offsets specified by BAT entries must meet the following requirements: [...] - the value must be lower than the desired file size,
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
On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote: > On 8/23/22 12:20, Denis V. Lunev wrote: >> On 23.08.2022 09:23, Alexander Ivanov 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"? >>>> >>>> It's hardly possible to get such image on valid scenarios with Qemu >>>> (keeping in mind bdrv_truncate() call in parallels_close()). But it >>>> still may be possible to have such images produced by another >>>> software or by some failure path. >>>> >>> I think you are right, it would be better to align image size up to >>> sector size. >> >> I would say that we need to align not on sector size but on cluster >> size. >> That would worth additional check. > > And not simply align, as data_offset is not necessarily aligned to > cluster size. > > Finally, what should we check? > > I suggest > > > diff --git a/block/parallels.c b/block/parallels.c > index 6d4ed77f16..b882ea1200 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -725,6 +725,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; > @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, > QDict *options, int flags, > return -EINVAL; > } > > + file_size = bdrv_getlength(bs->file->bs); > + if (file_size < 0) { > + return file_size; > + } > + > ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); > if (ret < 0) { > goto fail; > @@ -798,6 +804,13 @@ 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) { Like this, especially >= check which we have had missed. Though this would break the repair. We need additional if (flags & BDRV_O_CHECK) { continue; } No incorrect data_end assignment, which would be very welcome. Den > + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " > + "is larger than file size (%" PRIi64 ")", > + off, i, file_size); > + ret = -EINVAL; > + goto fail; > + } > if (off >= s->data_end) { > s->data_end = off + s->tracks; > } > > > > - better error message, and we check exactly what's written in the > spec (docs/interop/parallels.c): > > > Cluster offsets specified by BAT entries must meet the following > requirements: > [...] > - the value must be lower than the desired file size, > > >
On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote: > On 8/23/22 12:20, Denis V. Lunev wrote: >> On 23.08.2022 09:23, Alexander Ivanov 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"? >>>> >>>> It's hardly possible to get such image on valid scenarios with Qemu >>>> (keeping in mind bdrv_truncate() call in parallels_close()). But it >>>> still may be possible to have such images produced by another >>>> software or by some failure path. >>>> >>> I think you are right, it would be better to align image size up to >>> sector size. >> >> I would say that we need to align not on sector size but on cluster >> size. >> That would worth additional check. > > And not simply align, as data_offset is not necessarily aligned to > cluster size. > > Finally, what should we check? > > I suggest > > > diff --git a/block/parallels.c b/block/parallels.c > index 6d4ed77f16..b882ea1200 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -725,6 +725,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; > @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, > QDict *options, int flags, > return -EINVAL; > } > > + file_size = bdrv_getlength(bs->file->bs); > + if (file_size < 0) { > + return file_size; > + } > + > ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); > if (ret < 0) { > goto fail; > @@ -798,6 +804,13 @@ 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) { Do we really want to compare file size with cluster offset, not with cluster end? Also I would leave the comparison outside the loop, because I'm going to move highest offset calculation to a helper (there are two places with the same code). > + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " > + "is larger than file size (%" PRIi64 ")", > + off, i, file_size); > + ret = -EINVAL; > + goto fail; > + } > if (off >= s->data_end) { > s->data_end = off + s->tracks; > } > > > > - better error message, and we check exactly what's written in the > spec (docs/interop/parallels.c): > > > Cluster offsets specified by BAT entries must meet the following > requirements: > [...] > - the value must be lower than the desired file size, > > >
On 8/23/22 16:50, Alexander Ivanov wrote: > > On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote: >> On 8/23/22 12:20, Denis V. Lunev wrote: >>> On 23.08.2022 09:23, Alexander Ivanov 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"? >>>>> >>>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >>>>> >>>> I think you are right, it would be better to align image size up to sector size. >>> >>> I would say that we need to align not on sector size but on cluster size. >>> That would worth additional check. >> >> And not simply align, as data_offset is not necessarily aligned to cluster size. >> >> Finally, what should we check? >> >> I suggest >> >> >> diff --git a/block/parallels.c b/block/parallels.c >> index 6d4ed77f16..b882ea1200 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -725,6 +725,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; >> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >> return -EINVAL; >> } >> >> + file_size = bdrv_getlength(bs->file->bs); >> + if (file_size < 0) { >> + return file_size; >> + } >> + >> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); >> if (ret < 0) { >> goto fail; >> @@ -798,6 +804,13 @@ 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) { > > Do we really want to compare file size with cluster offset, not with cluster end? the spec say: Cluster offsets specified by BAT entries must meet the following requirements: [...] - the value must be lower than the desired file size, > > Also I would leave the comparison outside the loop, because I'm going to move highest offset calculation to a helper (there are two places with the same code). benefits of check inside the loop: 1. we get good error message with BAT index and problematic offset 2. we are sure that data_end is produced by real cluster. After the loop you'll have to consider image with no clusters separately > >> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " >> + "is larger than file size (%" PRIi64 ")", >> + off, i, file_size); >> + ret = -EINVAL; >> + goto fail; >> + } >> if (off >= s->data_end) { >> s->data_end = off + s->tracks; >> } >> >> >> >> - better error message, and we check exactly what's written in the spec (docs/interop/parallels.c): >> >> >> Cluster offsets specified by BAT entries must meet the following requirements: >> [...] >> - the value must be lower than the desired file size, >> >> >>
On 8/23/22 13:11, Denis V. Lunev wrote: > On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote: >> On 8/23/22 12:20, Denis V. Lunev wrote: >>> On 23.08.2022 09:23, Alexander Ivanov 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"? >>>>> >>>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path. >>>>> >>>> I think you are right, it would be better to align image size up to sector size. >>> >>> I would say that we need to align not on sector size but on cluster size. >>> That would worth additional check. >> >> And not simply align, as data_offset is not necessarily aligned to cluster size. >> >> Finally, what should we check? >> >> I suggest >> >> >> diff --git a/block/parallels.c b/block/parallels.c >> index 6d4ed77f16..b882ea1200 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -725,6 +725,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; >> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, >> return -EINVAL; >> } >> >> + file_size = bdrv_getlength(bs->file->bs); >> + if (file_size < 0) { >> + return file_size; >> + } >> + >> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph)); >> if (ret < 0) { >> goto fail; >> @@ -798,6 +804,13 @@ 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) { > Like this, especially >= check which we have had missed. > Though this would break the repair. We need additional > > if (flags & BDRV_O_CHECK) { > continue; > } > > No incorrect data_end assignment, which would be > very welcome. > > Den 'continue' here will change the logic around data_end. We'll drop "wrong" clusters from calculation of data_end, and should check, how it affects further logic. What about: for (i = 0; i < s->bat_size; i++) { int64_t off = bat2sect(s, i); if (off >= file_size && !(flags & BDRV_O_CHECK)) { error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " "is larger than file size (%" PRIi64 ")", off, i, file_size); ret = -EINVAL; goto fail; } if (off >= s->data_end) { s->data_end = off + s->tracks; } } - this we simply add new error-out on no-O_CHECK path. > >> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " >> + "is larger than file size (%" PRIi64 ")", >> + off, i, file_size); >> + ret = -EINVAL; >> + goto fail; >> + } >> if (off >= s->data_end) { >> s->data_end = off + s->tracks; >> } >> >> >> >> - better error message, and we check exactly what's written in the spec (docs/interop/parallels.c): >> >> >> Cluster offsets specified by BAT entries must meet the following requirements: >> [...] >> - the value must be lower than the desired file size, >> >> >> >
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 (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { /* Image was not closed correctly. The check is mandatory */ s->header_unclean = true;
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(+)