[Qemu-devel] [PATCH v1 0/8] Refactor DMG driver to have chunk size independence

[Qemu-devel] [PATCH v1 0/8] Refactor DMG driver to have chunk size independence

[Ashijeet Acharya]

This series helps to provide chunk size independence for DMG driver to prevent
denial-of-service in cases where untrusted files are being accessed by the user.

This task is mentioned on the public block ToDo
Here -> http://wiki.qemu.org/ToDo/Block/DmgChunkSizeIndependence

Patch 1 introduces a new data structure to aid caching of random access points
within a compressed stream.

Patch 2 is an extension of patch 1 and introduces a new function to
initialize/update/reset our cached random access point.

Patch 3 limits the output buffer size to a max of 2MB to avoid QEMU allocate
huge amounts of memory.

Patch 4 is a simple preparatory patch to aid handling of various types of chunks.

Patches 5 & 6 help to handle various types of chunks.

Patch 7 simply refactors dmg_co_preadv() to read multiple sectors at once.

Patch 8 finally removes the error messages QEMU used to throw when an image with
chunk sizes above 64MB were accessed by the user.

->Testing procedure:
Convert a DMG file to raw format using the "qemu-img convert" tool present in
v2.9.0
Next convert the same image again after applying these patches.
Compare the two images using "qemu-img compare" tool to check if they are
identical.

You can pickup any DMG image from the collection present
Here -> https://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03606.html

->Important note:
These patches assume that the terms "chunk" and "block" are synonyms of each other
when we talk about bz2 compressed streams. Thus according to the bz2 docs[1],
the max uncompressed size of a chunk/block can reach to 46MB which is less than
the previously allowed size of 64MB, so we can continue decompressing the whole
chunk/block at once instead of partial decompression just like we do now.

This limitation was forced by the fact that bz2 compressed streams do not allow
random access midway through a chunk/block as the BZ2_bzDecompress() API in bzlib
seeks for the magic key "BZh" before starting decompression.[2] This magic key is
present at the start of every chunk/block only and since our cached random access
points need not necessarily point to the start of a chunk/block, BZ2_bzDecompress()
fails with an error value BZ_DATA_ERROR_MAGIC[3]

[1] https://en.wikipedia.org/wiki/Bzip2#File_format
[2] https://blastedbio.blogspot.in/2011/11/random-access-to-bzip2.html
[3] http://linux.math.tifr.res.in/manuals/html/manual_3.html#SEC17

Special thanks to Peter Wu for helping me understand and tackle the bz2
compressed chunks.

Ashijeet Acharya (8):
  dmg: Introduce a new struct to cache random access points
  dmg: New function to help us cache random access point
  dmg: Limit the output buffer size to a max of 2MB
  dmg: Refactor and prepare dmg_read_chunk() to cache random access
    points
  dmg: Handle zlib compressed chunks
  dmg: Handle bz2 compressed/raw/zeroed chunks
  dmg: Refactor dmg_co_preadv() to start reading multiple sectors
  dmg: Remove the error messages to allow wild images

 block/dmg.c | 214 +++++++++++++++++++++++++++++++++++++++---------------------
 block/dmg.h |  10 +++
 2 files changed, 148 insertions(+), 76 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH v1 1/8] dmg: Introduce a new struct to cache random access points

[Ashijeet Acharya]

We need to cache the random access point while performing partial
decompression so that we can resume decompression from that point
onwards in our next sequential read request. Introduce a new struct
DMGReadState which will help us do this.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/dmg.h b/block/dmg.h
index b592d6f..ee67ae1 100644
--- a/block/dmg.h
+++ b/block/dmg.h
@@ -31,6 +31,15 @@
 #include "block/block_int.h"
 #include <zlib.h>
 
+/* used to cache current position in compressed input stream */
+typedef struct DMGReadState {
+    uint8_t *saved_next_in;
+    int64_t saved_avail_in;
+    int32_t saved_chunk_type;
+    int64_t sectors_read; /* possible sectors read in each cycle */
+    int32_t sector_offset_in_chunk;
+} DMGReadState;
+
 typedef struct BDRVDMGState {
     CoMutex lock;
     /* each chunk contains a certain number of sectors,
@@ -51,6 +60,7 @@ typedef struct BDRVDMGState {
     uint8_t *compressed_chunk;
     uint8_t *uncompressed_chunk;
     z_stream zstream;
+    DMGReadState *drs;
 } BDRVDMGState;
 
 extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
-- 
2.6.2

[Qemu-devel] [PATCH v1 2/8] dmg: New function to help us cache random access point

[Ashijeet Acharya]

Introduce a new cache_access_point() function which will help us first
cache a random access point inside a compressed stream and then keep
updating it according to our requirement at appropriate times.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index a7d25fc..c6fe8b0 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -128,6 +128,18 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
     }
 }
 
+static void cache_access_point(DMGReadState *drs, uint8_t *next_in,
+                                int64_t avail_in, int32_t chunk,
+                                int64_t sectors_read, int32_t sector_offset)
+{
+    drs->saved_next_in = next_in;
+    drs->saved_avail_in = avail_in;
+    drs->saved_chunk_type = chunk;
+    drs->sectors_read = sectors_read;
+    drs->sector_offset_in_chunk = sector_offset;
+    return;
+}
+
 static int64_t dmg_find_koly_offset(BdrvChild *file, Error **errp)
 {
     BlockDriverState *file_bs = file->bs;
@@ -507,6 +519,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    s->drs = g_malloc(sizeof(DMGReadState));
+    /* initialise our access point cache */
+    cache_access_point(s->drs, NULL, -1, -1, -1, -1);
+
     if (inflateInit(&s->zstream) != Z_OK) {
         ret = -EINVAL;
         goto fail;
@@ -523,6 +539,7 @@ fail:
     g_free(s->lengths);
     g_free(s->sectors);
     g_free(s->sectorcounts);
+    g_free(s->drs);
     qemu_vfree(s->compressed_chunk);
     qemu_vfree(s->uncompressed_chunk);
     return ret;
@@ -685,6 +702,7 @@ static void dmg_close(BlockDriverState *bs)
     g_free(s->lengths);
     g_free(s->sectors);
     g_free(s->sectorcounts);
+    g_free(s->drs);
     qemu_vfree(s->compressed_chunk);
     qemu_vfree(s->uncompressed_chunk);
 
-- 
2.6.2

[Qemu-devel] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[Ashijeet Acharya]

The size of the output buffer is limited to a maximum of 2MB so that
QEMU doesn't end up allocating huge amounts of memory while
decompressing compressed input streams.

2MB is an appropriate size because "qemu-img convert" has the same I/O
buffer size and the most important use case for DMG files is to be
compatible with qemu-img convert.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index c6fe8b0..7ae30e3 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -37,8 +37,8 @@ enum {
     /* Limit chunk sizes to prevent unreasonable amounts of memory being used
      * or truncating when converting to 32-bit types
      */
-    DMG_LENGTHS_MAX = 64 * 1024 * 1024, /* 64 MB */
-    DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
+    DMG_MAX_OUTPUT = 2 * 1024 * 1024, /* 2 MB */
+    DMG_SECTOR_MAX = DMG_MAX_OUTPUT / 512,
 };
 
 static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -260,10 +260,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
 
         /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
          * therefore be unbounded. */
-        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
             error_report("sector count %" PRIu64 " for chunk %" PRIu32
                          " is larger than max (%u)",
-                         s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
+                         s->sectorcounts[i], i, DMG_SECTOR_MAX);
             ret = -EINVAL;
             goto fail;
         }
@@ -275,10 +275,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* length in (compressed) data fork */
         s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
 
-        if (s->lengths[i] > DMG_LENGTHS_MAX) {
+        if (s->lengths[i] > DMG_MAX_OUTPUT) {
             error_report("length %" PRIu64 " for chunk %" PRIu32
                          " is larger than max (%u)",
-                         s->lengths[i], i, DMG_LENGTHS_MAX);
+                         s->lengths[i], i, DMG_MAX_OUTPUT);
             ret = -EINVAL;
             goto fail;
         }
-- 
2.6.2

[Qemu-devel] [PATCH v1 4/8] dmg: Refactor and prepare dmg_read_chunk() to cache random access points

[Ashijeet Acharya]

Refactor dmg_read_chunk() to start making use of the new DMGReadState
structure and do chunk and sector related calculations based on it.
Add a new argument "DMGReadState *drs" to it.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 153 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 91 insertions(+), 62 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 7ae30e3..dc356b0 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -578,78 +578,106 @@ static inline uint32_t search_chunk(BDRVDMGState *s, uint64_t sector_num)
     return s->n_chunks; /* error */
 }
 
-static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
+static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num,
+                                 DMGReadState *drs)
 {
     BDRVDMGState *s = bs->opaque;
 
+    int ret;
+    uint32_t sector_offset;
+    uint64_t sectors_read;
+    uint32_t chunk;
+
     if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
-        int ret;
-        uint32_t chunk = search_chunk(s, sector_num);
+        chunk = search_chunk(s, sector_num);
+    } else {
+        chunk = drs->saved_chunk_type;
+    }
 
-        if (chunk >= s->n_chunks) {
+    if (chunk >= s->n_chunks) {
+            return -1;
+    }
+
+    /* reset our access point cache if we had a change in current chunk */
+    if (chunk != drs->saved_chunk_type) {
+        cache_access_point(drs, NULL, -1, -1, -1, -1);
+    }
+
+    sector_offset = sector_num - s->sectors[chunk];
+
+    if ((s->sectorcounts[chunk] - sector_offset) > DMG_SECTOR_MAX) {
+        sectors_read = DMG_SECTOR_MAX;
+    } else {
+        sectors_read = s->sectorcounts[chunk] - sector_offset;
+    }
+
+    /* truncate sectors read if it exceeds the 2MB buffer of qemu-img
+     * convert */
+    if ((sector_num % DMG_SECTOR_MAX) + sectors_read > DMG_SECTOR_MAX) {
+        sectors_read = DMG_SECTOR_MAX - (sector_num % DMG_SECTOR_MAX);
+    }
+
+    s->current_chunk = s->n_chunks;
+
+    switch (s->types[chunk]) { /* block entry type */
+    case 0x80000005: { /* zlib compressed */
+        /* we need to buffer, because only the chunk as whole can be
+         * inflated. */
+        ret = bdrv_pread(bs->file, s->offsets[chunk],
+                         s->compressed_chunk, s->lengths[chunk]);
+        if (ret != s->lengths[chunk]) {
             return -1;
         }
 
-        s->current_chunk = s->n_chunks;
-        switch (s->types[chunk]) { /* block entry type */
-        case 0x80000005: { /* zlib compressed */
-            /* we need to buffer, because only the chunk as whole can be
-             * inflated. */
-            ret = bdrv_pread(bs->file, s->offsets[chunk],
-                             s->compressed_chunk, s->lengths[chunk]);
-            if (ret != s->lengths[chunk]) {
-                return -1;
-            }
-
-            s->zstream.next_in = s->compressed_chunk;
-            s->zstream.avail_in = s->lengths[chunk];
-            s->zstream.next_out = s->uncompressed_chunk;
-            s->zstream.avail_out = 512 * s->sectorcounts[chunk];
-            ret = inflateReset(&s->zstream);
-            if (ret != Z_OK) {
-                return -1;
-            }
-            ret = inflate(&s->zstream, Z_FINISH);
-            if (ret != Z_STREAM_END ||
-                s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
-                return -1;
-            }
-            break; }
-        case 0x80000006: /* bzip2 compressed */
-            if (!dmg_uncompress_bz2) {
-                break;
-            }
-            /* we need to buffer, because only the chunk as whole can be
-             * inflated. */
-            ret = bdrv_pread(bs->file, s->offsets[chunk],
-                             s->compressed_chunk, s->lengths[chunk]);
-            if (ret != s->lengths[chunk]) {
-                return -1;
-            }
-
-            ret = dmg_uncompress_bz2((char *)s->compressed_chunk,
-                                     (unsigned int) s->lengths[chunk],
-                                     (char *)s->uncompressed_chunk,
-                                     (unsigned int)
-                                         (512 * s->sectorcounts[chunk]));
-            if (ret < 0) {
-                return ret;
-            }
-            break;
-        case 1: /* copy */
-            ret = bdrv_pread(bs->file, s->offsets[chunk],
-                             s->uncompressed_chunk, s->lengths[chunk]);
-            if (ret != s->lengths[chunk]) {
-                return -1;
-            }
-            break;
-        case 2: /* zero */
-            /* see dmg_read, it is treated specially. No buffer needs to be
-             * pre-filled, the zeroes can be set directly. */
+        s->zstream.next_in = s->compressed_chunk;
+        s->zstream.avail_in = s->lengths[chunk];
+        s->zstream.next_out = s->uncompressed_chunk;
+        s->zstream.avail_out = 512 * s->sectorcounts[chunk];
+        ret = inflateReset(&s->zstream);
+        if (ret != Z_OK) {
+            return -1;
+        }
+        ret = inflate(&s->zstream, Z_FINISH);
+        if (ret != Z_STREAM_END ||
+            s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
+            return -1;
+        }
+        break; }
+    case 0x80000006: /* bzip2 compressed */
+        if (!dmg_uncompress_bz2) {
             break;
         }
-        s->current_chunk = chunk;
+        /* we need to buffer, because only the chunk as whole can be
+         * inflated. */
+        ret = bdrv_pread(bs->file, s->offsets[chunk],
+                         s->compressed_chunk, s->lengths[chunk]);
+        if (ret != s->lengths[chunk]) {
+            return -1;
+        }
+
+        ret = dmg_uncompress_bz2((char *)s->compressed_chunk,
+                                 (unsigned int) s->lengths[chunk],
+                                 (char *)s->uncompressed_chunk,
+                                 (unsigned int)
+                                    (512 * s->sectorcounts[chunk]));
+        if (ret < 0) {
+            return ret;
+        }
+        break;
+    case 1: /* copy */
+        ret = bdrv_pread(bs->file, s->offsets[chunk],
+                         s->uncompressed_chunk, s->lengths[chunk]);
+        if (ret != s->lengths[chunk]) {
+            return -1;
+        }
+        break;
+    case 2: /* zero */
+        /* see dmg_read, it is treated specially. No buffer needs to be
+         * pre-filled, the zeroes can be set directly. */
+        break;
     }
+    s->current_chunk = chunk;
+
     return 0;
 }
 
@@ -661,6 +689,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
     int ret, i;
+    DMGReadState *drs = s->drs;
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
@@ -671,7 +700,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         uint32_t sector_offset_in_chunk;
         void *data;
 
-        if (dmg_read_chunk(bs, sector_num + i) != 0) {
+        if (dmg_read_chunk(bs, sector_num + i, drs) != 0) {
             ret = -EIO;
             goto fail;
         }
-- 
2.6.2

[Qemu-devel] [PATCH v1 5/8] dmg: Handle zlib compressed chunks

[Ashijeet Acharya]

Set the output buffer size to be equal to the size of number of sectors
stored in @sectors_read. Start inflating to a max output buffer size of
2MB and cache our access point to aid random access later if required.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index dc356b0..749c151 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -621,27 +621,47 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num,
 
     switch (s->types[chunk]) { /* block entry type */
     case 0x80000005: { /* zlib compressed */
-        /* we need to buffer, because only the chunk as whole can be
-         * inflated. */
-        ret = bdrv_pread(bs->file, s->offsets[chunk],
-                         s->compressed_chunk, s->lengths[chunk]);
-        if (ret != s->lengths[chunk]) {
-            return -1;
+        /* check for cached random access point */
+        if (drs->saved_next_in == NULL) {
+            /* we need to buffer, because only the chunk as whole can be
+             * inflated. */
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
+                             s->compressed_chunk, s->lengths[chunk]);
+            if (ret != s->lengths[chunk]) {
+                return -1;
+            }
+
+            s->zstream.next_in = s->compressed_chunk;
+            s->zstream.avail_in = s->lengths[chunk];
+        } else {
+            s->zstream.next_in = drs->saved_next_in;
+            s->zstream.avail_in = drs->saved_avail_in;
         }
 
-        s->zstream.next_in = s->compressed_chunk;
-        s->zstream.avail_in = s->lengths[chunk];
         s->zstream.next_out = s->uncompressed_chunk;
-        s->zstream.avail_out = 512 * s->sectorcounts[chunk];
-        ret = inflateReset(&s->zstream);
-        if (ret != Z_OK) {
-            return -1;
+
+        s->zstream.avail_out = sectors_read * BDRV_SECTOR_SIZE;
+
+        if (drs->saved_next_in == NULL) {
+            ret = inflateReset(&s->zstream);
+            if (ret != Z_OK) {
+                return -1;
+            }
+        }
+        /* reset total_out for each successive call */
+        s->zstream.total_out = 0;
+        ret = inflate(&s->zstream, Z_SYNC_FLUSH);
+        if (ret == Z_OK &&
+            s->zstream.total_out == 512 * sectors_read) {
+            goto update;
         }
-        ret = inflate(&s->zstream, Z_FINISH);
         if (ret != Z_STREAM_END ||
-            s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
+            s->zstream.total_out != 512 * sectors_read) {
             return -1;
         }
+update:
+        cache_access_point(drs, s->zstream.next_in, s->zstream.avail_in,
+                           chunk, sectors_read, sector_offset);
         break; }
     case 0x80000006: /* bzip2 compressed */
         if (!dmg_uncompress_bz2) {
-- 
2.6.2

[Qemu-devel] [PATCH v1 6/8] dmg: Handle bz2 compressed/raw/zeroed chunks

[Ashijeet Acharya]

We do not need to cache the access point for these chunks but need to
update our various supporting variables like chunk, sectors_read etc.
to keep maintaining our code structure. Call cache_access_point() after
reading chunks of these types.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 749c151..f643e41 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -680,20 +680,30 @@ update:
                                  (char *)s->uncompressed_chunk,
                                  (unsigned int)
                                     (512 * s->sectorcounts[chunk]));
+
         if (ret < 0) {
             return ret;
         }
+        cache_access_point(drs, NULL, -1, chunk, sectors_read,
+                           sector_offset);
         break;
     case 1: /* copy */
-        ret = bdrv_pread(bs->file, s->offsets[chunk],
-                         s->uncompressed_chunk, s->lengths[chunk]);
-        if (ret != s->lengths[chunk]) {
-            return -1;
+        if (drs->sectors_read == -1) {
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
+                             s->uncompressed_chunk, s->lengths[chunk]);
+            if (ret != s->lengths[chunk]) {
+                return -1;
+            }
         }
+        cache_access_point(drs, NULL, -1, chunk, sectors_read,
+                           sector_offset);
         break;
     case 2: /* zero */
         /* see dmg_read, it is treated specially. No buffer needs to be
          * pre-filled, the zeroes can be set directly. */
+        cache_access_point(drs, NULL, -1, chunk, sectors_read,
+                           sector_offset);
+
         break;
     }
     s->current_chunk = chunk;
-- 
2.6.2

[Qemu-devel] [PATCH v1 7/8] dmg: Refactor dmg_co_preadv() to start reading multiple sectors

[Ashijeet Acharya]

At the moment, dmg_co_preadv() reads one sector at a time. Make it
read multiple sectors at a time depending on the number of sectors
stored in "drs->sectors_read". This does not provide any significant
optimization in the I/O process of DMG but is still a nicer way.

Adjust the 'data' variable depending on our cached access point
situation to align our read requests appropriately.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index f643e41..b0f3c84 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -718,7 +718,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     BDRVDMGState *s = bs->opaque;
     uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
-    int ret, i;
+    int ret, i = 0;
     DMGReadState *drs = s->drs;
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
@@ -726,8 +726,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
 
     qemu_co_mutex_lock(&s->lock);
 
-    for (i = 0; i < nb_sectors; i++) {
-        uint32_t sector_offset_in_chunk;
+    while (i < nb_sectors) {
         void *data;
 
         if (dmg_read_chunk(bs, sector_num + i, drs) != 0) {
@@ -738,12 +737,20 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
          * s->uncompressed_chunk may be too small to cover the large all-zeroes
          * section. dmg_read_chunk is called to find s->current_chunk */
         if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
-            qemu_iovec_memset(qiov, i * 512, 0, 512);
-            continue;
+            qemu_iovec_memset(qiov, i * 512, 0,
+                                    512 * drs->sectors_read);
+            goto increment;
+        }
+
+        if (drs->saved_next_in == NULL) {
+            data = s->uncompressed_chunk + drs->sector_offset_in_chunk * 512;
+        } else {
+            data = s->uncompressed_chunk;
         }
-        sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
-        data = s->uncompressed_chunk + sector_offset_in_chunk * 512;
-        qemu_iovec_from_buf(qiov, i * 512, data, 512);
+        qemu_iovec_from_buf(qiov, i * 512, data, drs->sectors_read * 512);
+
+increment:
+        i += drs->sectors_read;
     }
 
     ret = 0;
-- 
2.6.2

[Qemu-devel] [PATCH v1 8/8] dmg: Remove the error messages to allow wild images

[Ashijeet Acharya]

We have refactored the DMG driver to accept and process images
irrespective of their chunk sizes since we now have limit of 2MB on our
output buffer size. Thus QEMU will not allocate huge amounts of memory
no matter what the chunk size is.

Remove the error messages to prevent denial-of-service in cases where
untrusted files are being accessed by the user.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/dmg.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index b0f3c84..01ec40e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -209,7 +209,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
                                uint8_t *buffer, uint32_t count)
 {
     uint32_t type, i;
-    int ret;
     size_t new_size;
     uint32_t chunk_count;
     int64_t offset = 0;
@@ -258,16 +257,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* sector count */
         s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
-        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
-         * therefore be unbounded. */
-        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
-            error_report("sector count %" PRIu64 " for chunk %" PRIu32
-                         " is larger than max (%u)",
-                         s->sectorcounts[i], i, DMG_SECTOR_MAX);
-            ret = -EINVAL;
-            goto fail;
-        }
-
         /* offset in (compressed) data fork */
         s->offsets[i] = buff_read_uint64(buffer, offset + 0x18);
         s->offsets[i] += in_offset;
@@ -275,23 +264,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* length in (compressed) data fork */
         s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
 
-        if (s->lengths[i] > DMG_MAX_OUTPUT) {
-            error_report("length %" PRIu64 " for chunk %" PRIu32
-                         " is larger than max (%u)",
-                         s->lengths[i], i, DMG_MAX_OUTPUT);
-            ret = -EINVAL;
-            goto fail;
-        }
-
         update_max_chunk_size(s, i, &ds->max_compressed_size,
                               &ds->max_sectors_per_chunk);
         offset += 40;
     }
     s->n_chunks += chunk_count;
     return 0;
-
-fail:
-    return ret;
 }
 
 static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
-- 
2.6.2

Re: [Qemu-devel] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[John Snow]

On 04/25/2017 03:59 PM, Ashijeet Acharya wrote:
> The size of the output buffer is limited to a maximum of 2MB so that
> QEMU doesn't end up allocating huge amounts of memory while
> decompressing compressed input streams.
> 
> 2MB is an appropriate size because "qemu-img convert" has the same I/O
> buffer size and the most important use case for DMG files is to be
> compatible with qemu-img convert.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---

Patch 1 adds a new structure and patch 2 starts using it, but in a
store-only manner and only with placeholder variables that are difficult
to authenticate, so there's still "insufficient data" to review either
patch meaningfully.

This patch seems unrelated to either of those, so the ordering is strange.

>  block/dmg.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/dmg.c b/block/dmg.c
> index c6fe8b0..7ae30e3 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -37,8 +37,8 @@ enum {
>      /* Limit chunk sizes to prevent unreasonable amounts of memory being used
>       * or truncating when converting to 32-bit types
>       */
> -    DMG_LENGTHS_MAX = 64 * 1024 * 1024, /* 64 MB */
> -    DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
> +    DMG_MAX_OUTPUT = 2 * 1024 * 1024, /* 2 MB */

why "MAX OUTPUT" ? Aren't we using this for buffering on reads?

> +    DMG_SECTOR_MAX = DMG_MAX_OUTPUT / 512,
>  };
>  
>  static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
> @@ -260,10 +260,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>  
>          /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
>           * therefore be unbounded. */
> -        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> +        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
>              error_report("sector count %" PRIu64 " for chunk %" PRIu32
>                           " is larger than max (%u)",
> -                         s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> +                         s->sectorcounts[i], i, DMG_SECTOR_MAX);
>              ret = -EINVAL;
>              goto fail;
>          }
> @@ -275,10 +275,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>          /* length in (compressed) data fork */
>          s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
>  
> -        if (s->lengths[i] > DMG_LENGTHS_MAX) {
> +        if (s->lengths[i] > DMG_MAX_OUTPUT) {
>              error_report("length %" PRIu64 " for chunk %" PRIu32
>                           " is larger than max (%u)",
> -                         s->lengths[i], i, DMG_LENGTHS_MAX);
> +                         s->lengths[i], i, DMG_MAX_OUTPUT);
>              ret = -EINVAL;
>              goto fail;
>          }
> 

Seems OK otherwise, but I would normally expect you to fix the buffering
problems first, and then reduce the size of the buffer -- not the other
way around. This version introduces new limitations that didn't exist
previously (As of this commit, QEMU can't open DMG files with chunks
larger than 2MB now, right?)

--js

Re: [Qemu-devel] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[Ashijeet Acharya]

On Thu, Apr 27, 2017 at 3:00 AM, John Snow <jsnow@redhat.com> wrote:
>
>
> On 04/25/2017 03:59 PM, Ashijeet Acharya wrote:
>> The size of the output buffer is limited to a maximum of 2MB so that
>> QEMU doesn't end up allocating huge amounts of memory while
>> decompressing compressed input streams.
>>
>> 2MB is an appropriate size because "qemu-img convert" has the same I/O
>> buffer size and the most important use case for DMG files is to be
>> compatible with qemu-img convert.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>
> Patch 1 adds a new structure and patch 2 starts using it, but in a
> store-only manner and only with placeholder variables that are difficult
> to authenticate, so there's still "insufficient data" to review either
> patch meaningfully.
>
> This patch seems unrelated to either of those, so the ordering is strange.

Actually, I have tried to keep these patches very short so that it is
easier to review them (mainly because of the time limitation I have).
But seems like I over tried. If you have any suggestions for the first
2 patches, I am happy to change them in you preferred way.

>
>>  block/dmg.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/dmg.c b/block/dmg.c
>> index c6fe8b0..7ae30e3 100644
>> --- a/block/dmg.c
>> +++ b/block/dmg.c
>> @@ -37,8 +37,8 @@ enum {
>>      /* Limit chunk sizes to prevent unreasonable amounts of memory being used
>>       * or truncating when converting to 32-bit types
>>       */
>> -    DMG_LENGTHS_MAX = 64 * 1024 * 1024, /* 64 MB */
>> -    DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
>> +    DMG_MAX_OUTPUT = 2 * 1024 * 1024, /* 2 MB */
>
> why "MAX OUTPUT" ? Aren't we using this for buffering on reads?

I just thought that this looked better, but I will revert back to the
original one.

>
>> +    DMG_SECTOR_MAX = DMG_MAX_OUTPUT / 512,
>>  };
>>
>>  static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
>> @@ -260,10 +260,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>>
>>          /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
>>           * therefore be unbounded. */
>> -        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
>> +        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
>>              error_report("sector count %" PRIu64 " for chunk %" PRIu32
>>                           " is larger than max (%u)",
>> -                         s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
>> +                         s->sectorcounts[i], i, DMG_SECTOR_MAX);
>>              ret = -EINVAL;
>>              goto fail;
>>          }
>> @@ -275,10 +275,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>>          /* length in (compressed) data fork */
>>          s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
>>
>> -        if (s->lengths[i] > DMG_LENGTHS_MAX) {
>> +        if (s->lengths[i] > DMG_MAX_OUTPUT) {
>>              error_report("length %" PRIu64 " for chunk %" PRIu32
>>                           " is larger than max (%u)",
>> -                         s->lengths[i], i, DMG_LENGTHS_MAX);
>> +                         s->lengths[i], i, DMG_MAX_OUTPUT);
>>              ret = -EINVAL;
>>              goto fail;
>>          }
>>
>
> Seems OK otherwise, but I would normally expect you to fix the buffering
> problems first, and then reduce the size of the buffer -- not the other
> way around. This version introduces new limitations that didn't exist
> previously (As of this commit, QEMU can't open DMG files with chunks
> larger than 2MB now, right?)

I think I will squash it with the last one (patch 8) which removes
this limitation completely and will also fix the problem of handling
the buffering problems first and then reducing the buffer size?

Ashijeet

Re: [Qemu-devel] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[Fam Zheng]

On Wed, 04/26 17:30, John Snow wrote:
> Seems OK otherwise, but I would normally expect you to fix the buffering
> problems first, and then reduce the size of the buffer -- not the other
> way around. This version introduces new limitations that didn't exist
> previously (As of this commit, QEMU can't open DMG files with chunks
> larger than 2MB now, right?)

Yes, each commit should _not_ introduce issues (compiling failures, functional
degeneration, etc.), and cannot rely on following commits to fix things screwed
up in this one.

This is important for bisectability - each commit can be built and tested in the
whole git history.

Fam

Re: [Qemu-devel] [PATCH v1 3/8] dmg: Limit the output buffer size to a max of 2MB

[Ashijeet Acharya]

On Thu, Apr 27, 2017 at 12:56 PM, Fam Zheng <famz@redhat.com> wrote:
> On Wed, 04/26 17:30, John Snow wrote:
>> Seems OK otherwise, but I would normally expect you to fix the buffering
>> problems first, and then reduce the size of the buffer -- not the other
>> way around. This version introduces new limitations that didn't exist
>> previously (As of this commit, QEMU can't open DMG files with chunks
>> larger than 2MB now, right?)
>
> Yes, each commit should _not_ introduce issues (compiling failures, functional
> degeneration, etc.), and cannot rely on following commits to fix things screwed
> up in this one.
>
> This is important for bisectability - each commit can be built and tested in the
> whole git history.

Yes, understood. That's why I am gonna squash it with the last patch
(patch 8) which removes this limitation completely.

Ashijeet