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) {