[Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144)

[Stefan Weil <sw@weilnetz.de>]

(1) block_size must not be null.

(2) blocks_in_image * 4 must fit into a size_t.

(3) blocks_in_image * block_size must fit into a uint64_t.

Header field disk_size already has a bounds check which now works
because of modification (1) and (3).

This patch was inspired by Jeff Cody's patch for the same problem.

Cc: Jeff Cody <jcody@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Hello Stefan, hello Jeff,

I tried to improve your previous patch - maybe you want to improve it further
or take parts from it to fix that CVE (of which I have no information).

The patch was compiled on 32 and 64 bit Linux and cross compiled with MinGW-w64,
but not tested otherwise.

Regards
Stefan

 block/vdi.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index b832905..8fb59a1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
                    header.sector_size, SECTOR_SIZE);
         ret = -ENOTSUP;
         goto fail;
-#if !defined(CONFIG_VDI_BLOCK_SIZE)
+#if defined(CONFIG_VDI_BLOCK_SIZE)
+    } else if (header.block_size == 0) {
+        error_setg(errp, "unsupported VDI image (block size is null)");
+        ret = -EINVAL;
+        goto fail;
+#else
     } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
         error_setg(errp, "unsupported VDI image (block size %u is not %u)",
                    header.block_size, DEFAULT_CLUSTER_SIZE);
@@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
                    (uint64_t)header.blocks_in_image * header.block_size);
         ret = -ENOTSUP;
         goto fail;
+#if SIZE_MAX <= UINT32_MAX
+    } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) {
+        /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */
+        error_setg(errp, "unsupported VDI image (number of blocks %u, "
+                   "only %zu are possible)",
+                   header.blocks_in_image, SIZE_MAX / sizeof(uint32_t));
+#endif
+    } else if (header.blocks_in_image > UINT64_MAX / header.block_size) {
+        /* blocks_in_image * block_size must fit into uint64_t. */
+        error_setg(errp, "unsupported VDI image (number of blocks %u, "
+                   "only %" PRIu64 " are possible)",
+                   header.blocks_in_image, UINT64_MAX / header.block_size);
     } else if (!uuid_is_null(header.uuid_link)) {
         error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
         ret = -ENOTSUP;
@@ -691,6 +708,33 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
+#if defined(CONFIG_VDI_BLOCK_SIZE)
+    if (block_size == 0) {
+        return -EINVAL;
+    }
+#endif
+
+    if (bytes > UINT64_MAX - block_size) {
+        /* Overflow in calculation of blocks (see below). */
+        return -ENOTSUP;
+    }
+
+    /* We need enough blocks to store the given disk size,
+       so always round up. */
+    blocks = (bytes + block_size - 1) / block_size;
+
+#if SIZE_MAX <= UINT32_MAX
+    if (blocks > SIZE_MAX / sizeof(uint32_t)) {
+        /* blocks * sizeof(uint32_t) must fit into size_t. */
+        return -ENOTSUP;
+    }
+#endif
+
+    if (blocks > UINT64_MAX / block_size) {
+        /* blocks * block_size must fit into uint64_t. */
+        return -EINVAL;
+    }
+
     fd = qemu_open(filename,
                    O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
                    0644);
@@ -698,10 +742,6 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options,
         return -errno;
     }
 
-    /* We need enough blocks to store the given disk size,
-       so always round up. */
-    blocks = (bytes + block_size - 1) / block_size;
-
     bmap_size = blocks * sizeof(uint32_t);
     bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
 
-- 
1.7.10.4