block/vmdk: Fix various bugs in handling malicious image files

This set of patches fixes four reported bugs in vmdk's handling of potentially malicious image files:

#3843: if cluster_sectors is 0x200000 we can overflow an 'int' and try

        to g_malloc() 0xffffffff80000000

#4017: if cluster_sectors is 0 we will divide by zero and SIGFPE #4029: we can overflow a 32*64 multiply; we catch it a bit later on
so there are no ill-effects, but for robustness it's better to check before the multiply
#4571: we can read off the end of a block of memory when trying to
parse descriptors; worst case is that we read some junk from the heap into BlockDriverState::auto_backing_file and ::backing_file. I don't think this is an exploitable info leak into the guest.
#4571: if the input file specifies no extents, we hit an assert
in vmdk_co_get_info() rather than rejecting the file (mentioned in passing in the same issue as previous bug)

I think these all classify as "hardening bugs" rather than security issues, since they're just "self-denial-of-service" at worst.

Tested using qemu-iotests on the vmdk format.

thanks
-- PMM

Peter Maydell (5):
block/vmdk: Avoid assertion if image file has no extents block/vmdk: Avoid reading off the end of descriptor memory block/vmdk: Use larger types for byte counts in vmdk_read_extent() block/vmdk: Reject a zero cluster_sectors parameter block/vmdk: Avoid overflowing multiplication in vmdk_open_vmdk4()

 block/vmdk.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

2.43.0

On a malformed image file which has no extents in it, we will open the file but then fall over on an assertion in vmdk_co_get_info():

    $ cat bad.vmdk
    # Disk DescriptorFile
    version=1
    CID=fffffffe
    parentCID=ffffffff
    createType="vmfs"

    $ qemu-img info -f vmdk /tmp/zz9
    qemu-img: ../../block/vmdk.c:3104: int vmdk_co_get_info(BlockDriverState *, BlockDriverInfo *): Assertion `s->num_extents' failed.
    Aborted (core dumped)

(This was noted in passing in the report of #4571.)

---
 block/vmdk.c | 6 ++++++
 1 file changed, 6 insertions(+)

index b6e78a2b5d9..86059e1fa08 100644
@@ -1301,6 +1301,12 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
         extent->type = g_strdup(type);
     }
 
+    if (s->num_extents == 0) {
+        error_setg(errp, "Invalid file: no extent lines present");
+        ret = -EINVAL;
+        goto out;
+    }
+
     ret = 0;
     goto out;
 

2.43.0

In vmdk_parent_open() and vmdk_read_cid(), we read DESC_SIZE bytes from the image file into a malloc'd buffer, and then look through it for a keyword using strstr(). If found, we skip past the keyword and attempt to parse a value from the next part of the buffer. However, we failed to check whether the end of the keyword is right at the end of the buffer, and so a malicious image can cause us to read off the end when parsing the value part.

Check for this condition before attempting to parse the value.

The worst case here is that the image causes us to read some junk from the heap into the BlockDriverState backing_file and auto_backing_file fields; the image could control those anyway, and it's unlikely that this heap data could e.g. be leaked to the guest.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4571

---
 block/vmdk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 86059e1fa08..6e4e0423dc9 100644
@@ -337,6 +337,10 @@ vmdk_read_cid(BlockDriverState *bs, int parent, uint32_t *pcid)
         goto out;
     }
     p_name += cid_str_size;
+    if (p_name >= desc + DESC_SIZE) {
+        ret = -EINVAL;
+        goto out;
+    }
     if (sscanf(p_name, "%" SCNx32, &cid) != 1) {
         ret = -EINVAL;
         goto out;

@@ -508,6 +512,10 @@ static int GRAPH_RDLOCK vmdk_parent_open(BlockDriverState *bs)
         char *end_name;
 
         p_name += sizeof("parentFileNameHint") + 1;
+        if (p_name >= desc + DESC_SIZE + 1) {
+            ret = -EINVAL;
+            goto out;
+        }
         end_name = strchr(p_name, '\"');
         if (end_name == NULL) {
             ret = -EINVAL;

2.43.0

In vmdk_read_extent() we use 'int' for cluster_bytes and buf_bytes. However, cluster_bytes is extent->cluster_sectors * 512 and buf_bytes is cluster_bytes * 2. The maximum value for extent->cluster_sectors (enforced in vmdk_add_extent()) is 0x20_0000, and so the worst case for buf_bytes is 0x8000_0000, which overflows a signed int.

Make the variable types here int64_t, which matches the type used by bdrv_co_pread()'s bytes argument and by extent->cluster_sectors.

The worst case thing that a malicious VMDK image could do is cause us to try to g_malloc() 0xffffffff80000000, which will cause QEMU to abort(), so this is a self-denial-of-service only, and so a hardening bug rather than a security issue.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3843

---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6e4e0423dc9..bd18d32085f 100644
@@ -1940,7 +1940,7 @@ vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
                  int64_t offset_in_cluster, QEMUIOVector *qiov, int bytes)
 {
     int ret;
-    int cluster_bytes, buf_bytes;
+    int64_t cluster_bytes, buf_bytes;
     uint8_t *cluster_buf, *compressed_data;
     uint8_t *uncomp_buf;
     uint32_t data_len;

2.43.0

In vmdk_add_extent() we reject image files with an excessively large cluster_sectors parameter, but have no lower bound. In particular, if the cluster_sectors is 0 then we will set l1_entry_sectors also to 0, and later crash with a SIGFPE for division by zero.

Reject a zero cluster_sectors value, unless this is a flat extent (in which case cluster_sectors isn't relevant and the callsite always passes us zero).

The worst case here is a self-denial-of-service with a QEMU crash, so this is a hardening bug.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4017

---
 block/vmdk.c | 5 +++++
 1 file changed, 5 insertions(+)

index bd18d32085f..d7e23ae8098 100644
@@ -552,6 +552,11 @@ static int vmdk_add_extent(BlockDriverState *bs,
     BDRVVmdkState *s = bs->opaque;
     int64_t nb_sectors;
 
+    if (!flat && cluster_sectors == 0) {
+        error_setg(errp, "Invalid zero granularity, image may be corrupt");
+        return -EINVAL;
+    }
+
     if (cluster_sectors > 0x200000) {
         /* 0x200000 * 512Bytes = 1GB for one cluster is unrealistic */
         error_setg(errp, "Invalid granularity, image may be corrupt");

2.43.0

In vmdk_open_vmdk4() we set the uint32_t l1_entry_sectors with:
l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
* le64_to_cpu(header.granularity);

We have checked that num_gtes_per_gt is at most 512, but the header.granularity has not been checked at this point, and so the multiply can overflow the 32-bit variable. This overflow is harmless beacuse we're about to call vmdk_add_extent(), which will fail if the header.granularity (cluster_sectors) is more than 0x20_0000, but it's a potential trap for the future if code that relies on the l1_entry_sectors value gets added between where we set it and where we make that check.

Check for overlarge granularity at the point of multiplication so it's clear we can't overflow.

Cc: [email protected]
Reported-by: Tristan <https://gitlab.com/TristanInSec>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4029

---
 block/vmdk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index d7e23ae8098..a9bd8a4245f 100644
@@ -1052,7 +1052,10 @@ vmdk_open_vmdk4(BlockDriverState *bs, BdrvChild *file, int flags,
         error_setg(errp, "L2 table size too big");
         return -EINVAL;
     }
-
+    if (le64_to_cpu(header.granularity) > 0x200000) {
+        error_setg(errp, "Invalid granularity, image may be corrupt");
+        return -EFBIG;
+    }
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {

2.43.0

Am 03.11.2026 um 15:35 hat Peter Maydell geschrieben:

Thanks, applied to the block branch.

It would have been nice to add new qemu-iotests cases that can catch these bugs, maybe you still want to do that on top?

And just looking briefly at each of the places that are patched, it's immediately clear to me that some more fixes are needed. :-)

Kevin

I had a look at this, but then re-reading the bug reports I kind of lost the will to continue trying to dig through the AI generated report slop to try to extract something resembling a repro case that could be put into the iotests. I'm also generally dubious about the utility of adding regression tests for "we do something wrong on this specific invalid input" because it seems unlikely to me that we reintroduce the same bug again.

The one that was reasonably doable was the "no extent lines" bug.

index 49c58ad05c3..79fd4de1e7e 100755
@@ -100,6 +100,18 @@ RW 12582912 VMFS "dummy.vmdk" 1
 EOF
 _img_info
 
+echo
+echo "=== Testing malformed VMFS with no extent lines ==="
+cat >"$TEST_IMG" <<EOF
+# Disk DescriptorFile
+version=1
+CID=fffffffe
+parentCID=ffffffff
+createType="vmfs"
+
+EOF
+_img_info
+
 echo
 echo "=== Testing truncated sparse ==="
 _make_test_img -o "subformat=monolithicSparse" 100G

index 275ee7c7781..56bdc085862 100644
@@ -2040,6 +2040,9 @@ read 512/512 bytes at offset 1063004405760
 === Testing malformed VMFS extent description line ===
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW

12582912 VMFS "dummy.IMGFMT" 1

+=== Testing malformed VMFS with no extent lines === +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid file: no extent lines present
+
=== Testing truncated sparse ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated,
expecting at least 13172736 bytes

thanks
-- PMM