[Qemu-devel] [PATCH v3 00/10] vmdk: Input validation fixes

[Qemu-devel] [PATCH v3 00/10] vmdk: Input validation fixes

[Fam Zheng]

Fixes for VMDK sparse file opening. Header fields are checked before used for
memory allocation.

v3:
    00: Rebase to master.
    04: Drop unused line in test script.
    05: change vmdk_add_extent signature (uint64_t cluster_sectors).
    06: Fix num_gtes_per_gte.
    08: Use qemu_blockalign for whole_grain.
        Don't allocate memory for !bs->backing_hd case.

Fam Zheng (9):
  vmdk: Make VMDK3Header QEMU_PACKED
  vmdk: Make VmdkGrainMarker QEMU_PACKED
  vmdk: use unsigned values for on disk header fields
  qemu-iotests: add empty test case for vmdk
  vmdk: check granularity field in opening
  vmdk: check l2 table size when opening
  vmdk: check l1 size before opening image
  vmdk: use heap allocation for whole_grain
  vmdk: rename num_gtes_per_gte to num_gtes_per_gt

Stefan Hajnoczi (1):
  qemu-iotests: add poke_file utility function

 block/vmdk.c                 | 104 ++++++++++++++++++++++++++++++-------------
 tests/qemu-iotests/059       |  72 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/059.out   |  20 +++++++++
 tests/qemu-iotests/common.rc |   6 +++
 tests/qemu-iotests/group     |   1 +
 5 files changed, 172 insertions(+), 31 deletions(-)
 create mode 100755 tests/qemu-iotests/059
 create mode 100644 tests/qemu-iotests/059.out

-- 
1.8.3.4

[Qemu-devel] [PATCH v3 01/10] vmdk: Make VMDK3Header QEMU_PACKED

[Fam Zheng]

Although the fields are all uint32_t, it's best to make it consistent
that all on disk structures are QEMU_PACKED.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 3756333..8ff43b9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -62,7 +62,7 @@ typedef struct {
     uint32_t cylinders;
     uint32_t heads;
     uint32_t sectors_per_track;
-} VMDK3Header;
+} QEMU_PACKED VMDK3Header;
 
 typedef struct {
     uint32_t version;
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 02/10] vmdk: Make VmdkGrainMarker QEMU_PACKED

[Fam Zheng]

For consistency, make this on disk structure QEMU_PACKED.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 8ff43b9..7ebe36b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -131,7 +131,7 @@ typedef struct VmdkGrainMarker {
     uint64_t lba;
     uint32_t size;
     uint8_t  data[0];
-} VmdkGrainMarker;
+} QEMU_PACKED VmdkGrainMarker;
 
 enum {
     MARKER_END_OF_STREAM    = 0,
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 03/10] vmdk: use unsigned values for on disk header fields

[Fam Zheng]

The size and offset fields are all non-negative values, use uint64_t for
them to avoid getting negative in memory value by int overflow.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 7ebe36b..976b871 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -67,14 +67,14 @@ typedef struct {
 typedef struct {
     uint32_t version;
     uint32_t flags;
-    int64_t capacity;
-    int64_t granularity;
-    int64_t desc_offset;
-    int64_t desc_size;
-    int32_t num_gtes_per_gte;
-    int64_t rgd_offset;
-    int64_t gd_offset;
-    int64_t grain_offset;
+    uint64_t capacity;
+    uint64_t granularity;
+    uint64_t desc_offset;
+    uint64_t desc_size;
+    uint32_t num_gtes_per_gte;
+    uint64_t rgd_offset;
+    uint64_t gd_offset;
+    uint64_t grain_offset;
     char filler[1];
     char check_bytes[4];
     uint16_t compressAlgorithm;
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 04/10] qemu-iotests: add poke_file utility function

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

The new poke_file function sets bytes at an offset in a file given a
printf-style format string.  It can be used to corrupt an image file for
test coverage of error paths.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.rc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index e9ba358..5e077c3 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -34,6 +34,12 @@ dd()
    fi
 }
 
+# poke_file 'test.img' 512 '\xff\xfe'
+poke_file()
+{
+    printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
+}
+
 # we need common.config
 if [ "$iam" != "check" ]
 then
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 05/10] qemu-iotests: add empty test case for vmdk

[Fam Zheng]

Will add vmdk specific tests later here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/059     | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/059.out |  2 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 54 insertions(+)
 create mode 100755 tests/qemu-iotests/059
 create mode 100644 tests/qemu-iotests/059.out

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
new file mode 100755
index 0000000..9dc7f64
--- /dev/null
+++ b/tests/qemu-iotests/059
@@ -0,0 +1,51 @@
+#!/bin/bash
+#
+# Test case for vmdk
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=famz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests vmdk-specific low-level functionality
+_supported_fmt vmdk
+_supported_proto generic
+_supported_os Linux
+
+granularity_offset=16
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
new file mode 100644
index 0000000..4ca7f29
--- /dev/null
+++ b/tests/qemu-iotests/059.out
@@ -0,0 +1,2 @@
+QA output created by 059
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b1d03c7..93ace2e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -63,3 +63,4 @@
 054 rw auto
 055 rw auto
 056 rw auto backing
+059 rw auto
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 06/10] vmdk: check granularity field in opening

[Fam Zheng]

Granularity is used to calculate the cluster size and allocate r/w
buffer. Check the value from image before using it, so we don't abort()
for unbounded memory allocation.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 40 +++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/059     |  8 +++++++-
 tests/qemu-iotests/059.out |  6 ++++++
 3 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 976b871..dc7bbc2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -385,15 +385,22 @@ static int vmdk_parent_open(BlockDriverState *bs)
 
 /* Create and append extent to the extent array. Return the added VmdkExtent
  * address. return NULL if allocation failed. */
-static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
+static int vmdk_add_extent(BlockDriverState *bs,
                            BlockDriverState *file, bool flat, int64_t sectors,
                            int64_t l1_offset, int64_t l1_backup_offset,
                            uint32_t l1_size,
-                           int l2_size, unsigned int cluster_sectors)
+                           int l2_size, uint64_t cluster_sectors,
+                           VmdkExtent **new_extent)
 {
     VmdkExtent *extent;
     BDRVVmdkState *s = bs->opaque;
 
+    if (cluster_sectors > 0x200000) {
+        /* 0x200000 * 512Bytes = 1GB for one cluster is unrealistic */
+        error_report("invalid granularity, image may be corrupt");
+        return -EINVAL;
+    }
+
     s->extents = g_realloc(s->extents,
                               (s->num_extents + 1) * sizeof(VmdkExtent));
     extent = &s->extents[s->num_extents];
@@ -416,7 +423,10 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState *bs,
         extent->end_sector = extent->sectors;
     }
     bs->total_sectors = extent->end_sector;
-    return extent;
+    if (new_extent) {
+        *new_extent = extent;
+    }
+    return 0;
 }
 
 static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent)
@@ -475,12 +485,17 @@ static int vmdk_open_vmdk3(BlockDriverState *bs,
     if (ret < 0) {
         return ret;
     }
-    extent = vmdk_add_extent(bs,
+
+    ret = vmdk_add_extent(bs,
                              bs->file, false,
                              le32_to_cpu(header.disk_sectors),
                              le32_to_cpu(header.l1dir_offset) << 9,
                              0, 1 << 6, 1 << 9,
-                             le32_to_cpu(header.granularity));
+                             le32_to_cpu(header.granularity),
+                             &extent);
+    if (ret < 0) {
+        return ret;
+    }
     ret = vmdk_init_tables(bs, extent);
     if (ret) {
         /* free extent allocated by vmdk_add_extent */
@@ -580,13 +595,17 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
         l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
     }
-    extent = vmdk_add_extent(bs, file, false,
+    ret = vmdk_add_extent(bs, file, false,
                           le64_to_cpu(header.capacity),
                           le64_to_cpu(header.gd_offset) << 9,
                           l1_backup_offset,
                           l1_size,
                           le32_to_cpu(header.num_gtes_per_gte),
-                          le64_to_cpu(header.granularity));
+                          le64_to_cpu(header.granularity),
+                          &extent);
+    if (ret < 0) {
+        return ret;
+    }
     extent->compressed =
         le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
     extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER;
@@ -702,8 +721,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             /* FLAT extent */
             VmdkExtent *extent;
 
-            extent = vmdk_add_extent(bs, extent_file, true, sectors,
-                            0, 0, 0, 0, sectors);
+            ret = vmdk_add_extent(bs, extent_file, true, sectors,
+                            0, 0, 0, 0, sectors, &extent);
+            if (ret < 0) {
+                return ret;
+            }
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE")) {
             /* SPARSE extent */
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 9dc7f64..9545e82 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -43,7 +43,13 @@ _supported_fmt vmdk
 _supported_proto generic
 _supported_os Linux
 
-granularity_offset=16
+granularity_offset=20
+
+echo "=== Testing invalid granularity ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$granularity_offset" "\xff\xff\xff\xff\xff\xff\xff\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 4ca7f29..380ca3d 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -1,2 +1,8 @@
 QA output created by 059
+=== Testing invalid granularity ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+invalid granularity, image may be corrupt
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 07/10] vmdk: check l2 table size when opening

[Fam Zheng]

header.num_gtes_per_gte determines size for L2 table. Check for too big
value before using it. Limit to 512M entries (2GB per one L2 table).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 5 +++++
 tests/qemu-iotests/059     | 7 +++++++
 tests/qemu-iotests/059.out | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index dc7bbc2..cb60d6d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -585,6 +585,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
+    if (le32_to_cpu(header.num_gtes_per_gte) > 512) {
+        error_report("L2 table size too big");
+        return -EINVAL;
+    }
+
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 9545e82..301eaca 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -44,6 +44,7 @@ _supported_proto generic
 _supported_os Linux
 
 granularity_offset=20
+grain_table_size_offset=44
 
 echo "=== Testing invalid granularity ==="
 echo
@@ -51,6 +52,12 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$granularity_offset" "\xff\xff\xff\xff\xff\xff\xff\xff"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "=== Testing too big L2 table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$grain_table_size_offset" "\xff\xff\xff\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 380ca3d..583955f 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -5,4 +5,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 invalid granularity, image may be corrupt
 qemu-io: can't open device TEST_DIR/t.vmdk
 no file open, try 'help open'
+=== Testing too big L2 table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+L2 table size too big
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 08/10] vmdk: check l1 size before opening image

[Fam Zheng]

L1 table size is calculated from capacity, granularity and l2 table
size. If capacity is too big or later two are too small, the L1 table
will be too big to allocate in memory. Limit it to a reasonable range.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 8 ++++++++
 tests/qemu-iotests/059     | 8 ++++++++
 tests/qemu-iotests/059.out | 6 ++++++
 3 files changed, 22 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index cb60d6d..3bcab26 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -597,6 +597,14 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     }
     l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
                 / l1_entry_sectors;
+    if (l1_size > 512 * 1024 * 1024) {
+        /* although with big capacity and small l1_entry_sectors, we can get a
+         * big l1_size, we don't want unbounded value to allocate the table.
+         * Limit it to 512M, which is 16PB for default cluster and L2 table
+         * size */
+        error_report("L1 size too big");
+        return -EFBIG;
+    }
     if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
         l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
     }
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 301eaca..b03429d 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -43,6 +43,7 @@ _supported_fmt vmdk
 _supported_proto generic
 _supported_os Linux
 
+capacity_offset=16
 granularity_offset=20
 grain_table_size_offset=44
 
@@ -58,6 +59,13 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$grain_table_size_offset" "\xff\xff\xff\xff"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo "=== Testing too big L1 table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$capacity_offset" "\xff\xff\xff\xff"
+poke_file "$TEST_IMG" "$grain_table_size_offset" "\x01\x00\x00\x00"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 583955f..9e715e5 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -11,4 +11,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 L2 table size too big
 qemu-io: can't open device TEST_DIR/t.vmdk
 no file open, try 'help open'
+=== Testing too big L1 table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+L1 size too big
+qemu-io: can't open device TEST_DIR/t.vmdk
+no file open, try 'help open'
 *** done
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 09/10] vmdk: use heap allocation for whole_grain

[Fam Zheng]

We should never grow the stack beyond 1 MB, otherwise we'll fall off the
end.  Thread stacks and coroutine stacks (1 MB) do not grow.
get_cluster_offset() allocates a big stack offset, it will fail for big
cluster images, change to heap allocated buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 3bcab26..21610e3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -842,16 +842,19 @@ static int get_whole_cluster(BlockDriverState *bs,
                 uint64_t offset,
                 bool allocate)
 {
-    /* 128 sectors * 512 bytes each = grain size 64KB */
-    uint8_t  whole_grain[extent->cluster_sectors * 512];
+    int ret = VMDK_OK;
+    uint8_t *whole_grain = NULL;
 
     /* we will be here if it's first write on non-exist grain(cluster).
      * try to read from parent image, if exist */
     if (bs->backing_hd) {
         int ret;
 
+        whole_grain =
+            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
         if (!vmdk_is_cid_valid(bs)) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* floor offset to cluster */
@@ -859,17 +862,21 @@ static int get_whole_cluster(BlockDriverState *bs,
         ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* Write grain only into the active image */
         ret = bdrv_write(extent->file, cluster_offset, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
     }
-    return VMDK_OK;
+exit:
+    qemu_vfree(whole_grain);
+    return ret;
 }
 
 static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
-- 
1.8.3.4

[Qemu-devel] [PATCH v3 10/10] vmdk: rename num_gtes_per_gte to num_gtes_per_gt

[Fam Zheng]

num_gtes_per_gte is a historical typo, rename it to a more sensible
name. It means "number of GrainTableEntries per GrainTable".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 21610e3..cb34b9f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -71,7 +71,7 @@ typedef struct {
     uint64_t granularity;
     uint64_t desc_offset;
     uint64_t desc_size;
-    uint32_t num_gtes_per_gte;
+    uint32_t num_gtes_per_gt;
     uint64_t rgd_offset;
     uint64_t gd_offset;
     uint64_t grain_offset;
@@ -585,12 +585,12 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
-    if (le32_to_cpu(header.num_gtes_per_gte) > 512) {
+    if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
         error_report("L2 table size too big");
         return -EINVAL;
     }
 
-    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
+    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
         return -EINVAL;
@@ -613,7 +613,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
                           le64_to_cpu(header.gd_offset) << 9,
                           l1_backup_offset,
                           l1_size,
-                          le32_to_cpu(header.num_gtes_per_gte),
+                          le32_to_cpu(header.num_gtes_per_gt),
                           le64_to_cpu(header.granularity),
                           &extent);
     if (ret < 0) {
@@ -1409,12 +1409,12 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
     header.capacity = filesize / 512;
     header.granularity = 128;
-    header.num_gtes_per_gte = 512;
+    header.num_gtes_per_gt = 512;
 
     grains = (filesize / 512 + header.granularity - 1) / header.granularity;
-    gt_size = ((header.num_gtes_per_gte * sizeof(uint32_t)) + 511) >> 9;
+    gt_size = ((header.num_gtes_per_gt * sizeof(uint32_t)) + 511) >> 9;
     gt_count =
-        (grains + header.num_gtes_per_gte - 1) / header.num_gtes_per_gte;
+        (grains + header.num_gtes_per_gt - 1) / header.num_gtes_per_gt;
     gd_size = (gt_count * sizeof(uint32_t) + 511) >> 9;
 
     header.desc_offset = 1;
@@ -1430,7 +1430,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     header.flags = cpu_to_le32(header.flags);
     header.capacity = cpu_to_le64(header.capacity);
     header.granularity = cpu_to_le64(header.granularity);
-    header.num_gtes_per_gte = cpu_to_le32(header.num_gtes_per_gte);
+    header.num_gtes_per_gt = cpu_to_le32(header.num_gtes_per_gt);
     header.desc_offset = cpu_to_le64(header.desc_offset);
     header.desc_size = cpu_to_le64(header.desc_size);
     header.rgd_offset = cpu_to_le64(header.rgd_offset);
-- 
1.8.3.4

Re: [Qemu-devel] [PATCH v3 02/10] vmdk: Make VmdkGrainMarker QEMU_PACKED

[Jeff Cody]

On Tue, Aug 06, 2013 at 09:40:35AM +0800, Fam Zheng wrote:
> For consistency, make this on disk structure QEMU_PACKED.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

I don't think this makes it necessary for a respin, but if you do one
anyway, I would suggest squashing this patch and patch 1 together.

> diff --git a/block/vmdk.c b/block/vmdk.c
> index 8ff43b9..7ebe36b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -131,7 +131,7 @@ typedef struct VmdkGrainMarker {
>      uint64_t lba;
>      uint32_t size;
>      uint8_t  data[0];
> -} VmdkGrainMarker;
> +} QEMU_PACKED VmdkGrainMarker;
>  
>  enum {
>      MARKER_END_OF_STREAM    = 0,
> -- 
> 1.8.3.4
> 

Re: [Qemu-devel] [PATCH v3 03/10] vmdk: use unsigned values for on disk header fields

[Jeff Cody]

On Tue, Aug 06, 2013 at 09:40:36AM +0800, Fam Zheng wrote:
> The size and offset fields are all non-negative values, use uint64_t for
> them to avoid getting negative in memory value by int overflow.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 7ebe36b..976b871 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -67,14 +67,14 @@ typedef struct {
>  typedef struct {
>      uint32_t version;
>      uint32_t flags;
> -    int64_t capacity;
> -    int64_t granularity;
> -    int64_t desc_offset;
> -    int64_t desc_size;
> -    int32_t num_gtes_per_gte;
> -    int64_t rgd_offset;
> -    int64_t gd_offset;
> -    int64_t grain_offset;
> +    uint64_t capacity;
> +    uint64_t granularity;
> +    uint64_t desc_offset;

desc_offset in BDRVVmdkState should be uint64_t as well, right?

> +    uint64_t desc_size;
> +    uint32_t num_gtes_per_gte;
> +    uint64_t rgd_offset;
> +    uint64_t gd_offset;
> +    uint64_t grain_offset;
>      char filler[1];
>      char check_bytes[4];
>      uint16_t compressAlgorithm;
> -- 
> 1.8.3.4
> 

Re: [Qemu-devel] [PATCH v3 03/10] vmdk: use unsigned values for on disk header fields

[Fam Zheng]

On Mon, 08/05 23:30, Jeff Cody wrote:
> On Tue, Aug 06, 2013 at 09:40:36AM +0800, Fam Zheng wrote:
> > The size and offset fields are all non-negative values, use uint64_t for
> > them to avoid getting negative in memory value by int overflow.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/vmdk.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 7ebe36b..976b871 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -67,14 +67,14 @@ typedef struct {
> >  typedef struct {
> >      uint32_t version;
> >      uint32_t flags;
> > -    int64_t capacity;
> > -    int64_t granularity;
> > -    int64_t desc_offset;
> > -    int64_t desc_size;
> > -    int32_t num_gtes_per_gte;
> > -    int64_t rgd_offset;
> > -    int64_t gd_offset;
> > -    int64_t grain_offset;
> > +    uint64_t capacity;
> > +    uint64_t granularity;
> > +    uint64_t desc_offset;
> 
> desc_offset in BDRVVmdkState should be uint64_t as well, right?

Yes, will fix.
> 
> > +    uint64_t desc_size;
> > +    uint32_t num_gtes_per_gte;
> > +    uint64_t rgd_offset;
> > +    uint64_t gd_offset;
> > +    uint64_t grain_offset;
> >      char filler[1];
> >      char check_bytes[4];
> >      uint16_t compressAlgorithm;
> > -- 
> > 1.8.3.4
> > 

-- 
Fam

Re: [Qemu-devel] [PATCH v3 09/10] vmdk: use heap allocation for whole_grain

[Jeff Cody]

On Tue, Aug 06, 2013 at 09:40:42AM +0800, Fam Zheng wrote:
> We should never grow the stack beyond 1 MB, otherwise we'll fall off the
> end.  Thread stacks and coroutine stacks (1 MB) do not grow.
> get_cluster_offset() allocates a big stack offset, it will fail for big
> cluster images, change to heap allocated buffer.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3bcab26..21610e3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -842,16 +842,19 @@ static int get_whole_cluster(BlockDriverState *bs,
>                  uint64_t offset,
>                  bool allocate)
>  {
> -    /* 128 sectors * 512 bytes each = grain size 64KB */
> -    uint8_t  whole_grain[extent->cluster_sectors * 512];
> +    int ret = VMDK_OK;
> +    uint8_t *whole_grain = NULL;
>  
>      /* we will be here if it's first write on non-exist grain(cluster).
>       * try to read from parent image, if exist */
>      if (bs->backing_hd) {
>          int ret;
            ^^^^^^
This above line should be removed; as it is this function will always return
VMDK_OK with this here, due to the scoping of this 'ret' inside the
if() block.

>  
> +        whole_grain =
> +            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
>          if (!vmdk_is_cid_valid(bs)) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>  
>          /* floor offset to cluster */
> @@ -859,17 +862,21 @@ static int get_whole_cluster(BlockDriverState *bs,
>          ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
>                  extent->cluster_sectors);
>          if (ret < 0) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>  
>          /* Write grain only into the active image */
>          ret = bdrv_write(extent->file, cluster_offset, whole_grain,
>                  extent->cluster_sectors);
>          if (ret < 0) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>      }
> -    return VMDK_OK;
> +exit:
> +    qemu_vfree(whole_grain);
> +    return ret;
>  }
>  
>  static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
> -- 
> 1.8.3.4
> 

Re: [Qemu-devel] [PATCH v3 10/10] vmdk: rename num_gtes_per_gte to num_gtes_per_gt

[Jeff Cody]

On Tue, Aug 06, 2013 at 09:40:43AM +0800, Fam Zheng wrote:
> num_gtes_per_gte is a historical typo, rename it to a more sensible
> name. It means "number of GrainTableEntries per GrainTable".

Could you put this as a comment as well above/next to the declaration
of num_gtes_per_gt?  It is hard not to see 'gtes' and read 'gates',
but a comment would clear that up.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 21610e3..cb34b9f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -71,7 +71,7 @@ typedef struct {
>      uint64_t granularity;
>      uint64_t desc_offset;
>      uint64_t desc_size;
> -    uint32_t num_gtes_per_gte;
> +    uint32_t num_gtes_per_gt;
>      uint64_t rgd_offset;
>      uint64_t gd_offset;
>      uint64_t grain_offset;
> @@ -585,12 +585,12 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>          return -ENOTSUP;
>      }
>  
> -    if (le32_to_cpu(header.num_gtes_per_gte) > 512) {
> +    if (le32_to_cpu(header.num_gtes_per_gt) > 512) {
>          error_report("L2 table size too big");
>          return -EINVAL;
>      }
>  
> -    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
> +    l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
>                          * le64_to_cpu(header.granularity);
>      if (l1_entry_sectors == 0) {
>          return -EINVAL;
> @@ -613,7 +613,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>                            le64_to_cpu(header.gd_offset) << 9,
>                            l1_backup_offset,
>                            l1_size,
> -                          le32_to_cpu(header.num_gtes_per_gte),
> +                          le32_to_cpu(header.num_gtes_per_gt),
>                            le64_to_cpu(header.granularity),
>                            &extent);
>      if (ret < 0) {
> @@ -1409,12 +1409,12 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>      header.compressAlgorithm = compress ? VMDK4_COMPRESSION_DEFLATE : 0;
>      header.capacity = filesize / 512;
>      header.granularity = 128;
> -    header.num_gtes_per_gte = 512;
> +    header.num_gtes_per_gt = 512;
>  
>      grains = (filesize / 512 + header.granularity - 1) / header.granularity;
> -    gt_size = ((header.num_gtes_per_gte * sizeof(uint32_t)) + 511) >> 9;
> +    gt_size = ((header.num_gtes_per_gt * sizeof(uint32_t)) + 511) >> 9;
>      gt_count =
> -        (grains + header.num_gtes_per_gte - 1) / header.num_gtes_per_gte;
> +        (grains + header.num_gtes_per_gt - 1) / header.num_gtes_per_gt;
>      gd_size = (gt_count * sizeof(uint32_t) + 511) >> 9;
>  
>      header.desc_offset = 1;
> @@ -1430,7 +1430,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>      header.flags = cpu_to_le32(header.flags);
>      header.capacity = cpu_to_le64(header.capacity);
>      header.granularity = cpu_to_le64(header.granularity);
> -    header.num_gtes_per_gte = cpu_to_le32(header.num_gtes_per_gte);
> +    header.num_gtes_per_gt = cpu_to_le32(header.num_gtes_per_gt);
>      header.desc_offset = cpu_to_le64(header.desc_offset);
>      header.desc_size = cpu_to_le64(header.desc_size);
>      header.rgd_offset = cpu_to_le64(header.rgd_offset);
> -- 
> 1.8.3.4
> 

Re: [Qemu-devel] [PATCH v3 00/10] vmdk: Input validation fixes

[Stefan Hajnoczi]

On Tue, Aug 06, 2013 at 09:40:33AM +0800, Fam Zheng wrote:
> Fixes for VMDK sparse file opening. Header fields are checked before used for
> memory allocation.
> 
> v3:
>     00: Rebase to master.
>     04: Drop unused line in test script.
>     05: change vmdk_add_extent signature (uint64_t cluster_sectors).
>     06: Fix num_gtes_per_gte.
>     08: Use qemu_blockalign for whole_grain.
>         Don't allocate memory for !bs->backing_hd case.
> 
> Fam Zheng (9):
>   vmdk: Make VMDK3Header QEMU_PACKED
>   vmdk: Make VmdkGrainMarker QEMU_PACKED
>   vmdk: use unsigned values for on disk header fields
>   qemu-iotests: add empty test case for vmdk
>   vmdk: check granularity field in opening
>   vmdk: check l2 table size when opening
>   vmdk: check l1 size before opening image
>   vmdk: use heap allocation for whole_grain
>   vmdk: rename num_gtes_per_gte to num_gtes_per_gt
> 
> Stefan Hajnoczi (1):
>   qemu-iotests: add poke_file utility function
> 
>  block/vmdk.c                 | 104 ++++++++++++++++++++++++++++++-------------
>  tests/qemu-iotests/059       |  72 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/059.out   |  20 +++++++++
>  tests/qemu-iotests/common.rc |   6 +++
>  tests/qemu-iotests/group     |   1 +
>  5 files changed, 172 insertions(+), 31 deletions(-)
>  create mode 100755 tests/qemu-iotests/059
>  create mode 100644 tests/qemu-iotests/059.out

I'm happy now modulo Jeff's comments.