(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 <jc...@redhat.com>
Cc: Kevin Wolf <kw...@redhat.com>
Cc: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Stefan Weil <s...@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
