| Message ID | 20231124115654.3239137-1-fam@euphon.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vmdk: Don't corrupt desc file in vmdk_write_cid | expand |
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?
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
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
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