[PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[Fam]

From: Fam Zheng <fam@euphon.net>

If the text description file is larger than DESC_SIZE, we force the last
byte in the buffer to be 0 and write it out.

This results in a corruption.

Try to allocate a big buffer in this case.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923

Signed-off-by: Fam Zheng <fam@euphon.net>
---
 block/vmdk.c               | 28 ++++++++++++++++++++--------
 tests/qemu-iotests/059     |  2 ++
 tests/qemu-iotests/059.out |  4 ++++
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index d87f6d9aaa..d6971c7067 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -351,29 +351,41 @@ vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     BDRVVmdkState *s = bs->opaque;
     int ret = 0;
 
-    desc = g_malloc0(DESC_SIZE);
-    tmp_desc = g_malloc0(DESC_SIZE);
-    ret = bdrv_co_pread(bs->file, s->desc_offset, DESC_SIZE, desc, 0);
+    size_t desc_buf_size;
+
+    if (s->desc_offset == 0) {
+        desc_buf_size = bdrv_getlength(bs->file->bs);
+        if (desc_buf_size > 16ULL << 20) {
+            error_report("VMDK description file too big");
+            return -EFBIG;
+        }
+    } else {
+        desc_buf_size = DESC_SIZE;
+    }
+
+    desc = g_malloc0(desc_buf_size);
+    tmp_desc = g_malloc0(desc_buf_size);
+    ret = bdrv_co_pread(bs->file, s->desc_offset, desc_buf_size, desc, 0);
     if (ret < 0) {
         goto out;
     }
 
-    desc[DESC_SIZE - 1] = '\0';
+    desc[desc_buf_size - 1] = '\0';
     tmp_str = strstr(desc, "parentCID");
     if (tmp_str == NULL) {
         ret = -EINVAL;
         goto out;
     }
 
-    pstrcpy(tmp_desc, DESC_SIZE, tmp_str);
+    pstrcpy(tmp_desc, desc_buf_size, tmp_str);
     p_name = strstr(desc, "CID");
     if (p_name != NULL) {
         p_name += sizeof("CID");
-        snprintf(p_name, DESC_SIZE - (p_name - desc), "%" PRIx32 "\n", cid);
-        pstrcat(desc, DESC_SIZE, tmp_desc);
+        snprintf(p_name, desc_buf_size - (p_name - desc), "%" PRIx32 "\n", cid);
+        pstrcat(desc, desc_buf_size, tmp_desc);
     }
 
-    ret = bdrv_co_pwrite_sync(bs->file, s->desc_offset, DESC_SIZE, desc, 0);
+    ret = bdrv_co_pwrite_sync(bs->file, s->desc_offset, desc_buf_size, desc, 0);
 
 out:
     g_free(desc);
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 2bcb1f7f9c..0634c7bd92 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -84,6 +84,8 @@ echo
 echo "=== Testing big twoGbMaxExtentFlat ==="
 _make_test_img -o "subformat=twoGbMaxExtentFlat" 1000G
 _img_info --format-specific | _filter_img_info --format-specific
+$QEMU_IO -c "write 990G 512 -P 89" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read 990G 512 -P 89" "$TEST_IMG" | _filter_qemu_io
 _cleanup_test_img
 
 echo
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 2b83c0c8b6..275ee7c778 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2032,6 +2032,10 @@ Format specific information:
             virtual size: 2147483648
             filename: TEST_DIR/t-f500.IMGFMT
             format: FLAT
+wrote 512/512 bytes at offset 1063004405760
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1063004405760
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing malformed VMFS extent description line ===
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
-- 
2.34.1

Re: [PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[Eric Blake]

On Fri, Nov 24, 2023 at 11:56:54AM +0000, Fam wrote:
> From: Fam Zheng <fam@euphon.net>
> 
> If the text description file is larger than DESC_SIZE, we force the last
> byte in the buffer to be 0 and write it out.
> 
> This results in a corruption.
> 
> Try to allocate a big buffer in this case.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923
> 
> Signed-off-by: Fam Zheng <fam@euphon.net>
> ---
>  block/vmdk.c               | 28 ++++++++++++++++++++--------
>  tests/qemu-iotests/059     |  2 ++
>  tests/qemu-iotests/059.out |  4 ++++
>  3 files changed, 26 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Are we trying to get this into 8.2, since it is a data corruption?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[Kevin Wolf]

Am 24.11.2023 um 12:56 hat Fam geschrieben:
> From: Fam Zheng <fam@euphon.net>
> 
> If the text description file is larger than DESC_SIZE, we force the last
> byte in the buffer to be 0 and write it out.
> 
> This results in a corruption.
> 
> Try to allocate a big buffer in this case.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923
> 
> Signed-off-by: Fam Zheng <fam@euphon.net>

Thanks, applied to the block branch.

But while I'm looking at this function, is there really anything that
guarantees that "parentCID" always exists and comes immediately after
"CID"? This looks like a questionable assumption to me.

Kevin

Re: [PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[Kevin Wolf]

Am 27.11.2023 um 16:18 hat Eric Blake geschrieben:
> On Fri, Nov 24, 2023 at 11:56:54AM +0000, Fam wrote:
> > From: Fam Zheng <fam@euphon.net>
> > 
> > If the text description file is larger than DESC_SIZE, we force the last
> > byte in the buffer to be 0 and write it out.
> > 
> > This results in a corruption.
> > 
> > Try to allocate a big buffer in this case.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923
> > 
> > Signed-off-by: Fam Zheng <fam@euphon.net>
> > ---
> >  block/vmdk.c               | 28 ++++++++++++++++++++--------
> >  tests/qemu-iotests/059     |  2 ++
> >  tests/qemu-iotests/059.out |  4 ++++
> >  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Are we trying to get this into 8.2, since it is a data corruption?

Yes, I've queued it for -rc2.

Kevin