[Qemu-devel] [PATCH v2 0/5] qcow1: Input validation fixes

[Qemu-devel] [PATCH v2 0/5] qcow1: Input validation fixes

[Kevin Wolf]

v2:
- Moved offset_l2_bits definition to patch 3 (Benoît)
- Added test cases for corner case values min-1 and max+1 (Benoît)

Kevin Wolf (5):
  qcow1: Make padding in the header explicit
  qcow1: Check maximum cluster size
  qcow1: Validate L2 table size (CVE-2014-0222)
  qcow1: Validate image size (CVE-2014-0223)
  qcow1: Stricter backing file length check

 block/qcow.c               | 44 +++++++++++++++++----
 tests/qemu-iotests/092     | 98 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/092.out | 38 ++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 174 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/092
 create mode 100644 tests/qemu-iotests/092.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/5] qcow1: Make padding in the header explicit

[Kevin Wolf]

We were relying on all compilers inserting the same padding in the
header struct that is used for the on-disk format. Let's not do that.
Mark the struct as packed and insert an explicit padding field for
compatibility.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block/qcow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow.c b/block/qcow.c
index 937dd6d..3684794 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -48,9 +48,10 @@ typedef struct QCowHeader {
     uint64_t size; /* in bytes */
     uint8_t cluster_bits;
     uint8_t l2_bits;
+    uint16_t padding;
     uint32_t crypt_method;
     uint64_t l1_table_offset;
-} QCowHeader;
+} QEMU_PACKED QCowHeader;
 
 #define L2_CACHE_SIZE 16
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/5] qcow1: Check maximum cluster size

[Kevin Wolf]

Huge values for header.cluster_bits cause unbounded allocations (e.g.
for s->cluster_cache) and crash qemu this way. Less huge values may
survive those allocations, but can cause integer overflows later on.

The only cluster sizes that qemu can create are 4k (for standalone
images) and 512 (for images with backing files), so we can limit it
to 64k.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c               | 10 ++++++--
 tests/qemu-iotests/092     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/092.out | 13 ++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/092
 create mode 100644 tests/qemu-iotests/092.out

diff --git a/block/qcow.c b/block/qcow.c
index 3684794..e60df23 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -128,11 +128,17 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    if (header.size <= 1 || header.cluster_bits < 9) {
-        error_setg(errp, "invalid value in qcow header");
+    if (header.size <= 1) {
+        error_setg(errp, "Image size is too small (must be at least 2 bytes)");
         ret = -EINVAL;
         goto fail;
     }
+    if (header.cluster_bits < 9 || header.cluster_bits > 16) {
+        error_setg(errp, "Cluster size must be between 512 and 64k");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     if (header.crypt_method > QCOW_CRYPT_AES) {
         error_setg(errp, "invalid encryption method in qcow header");
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
new file mode 100755
index 0000000..d060e6f
--- /dev/null
+++ b/tests/qemu-iotests/092
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# qcow1 format input validation tests
+#
+# Copyright (C) 2014 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=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm -f $TEST_IMG.snap
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow
+_supported_proto generic
+_supported_os Linux
+
+offset_cluster_bits=32
+
+echo
+echo "== Invalid cluster size =="
+_make_test_img 64M
+poke_file "$TEST_IMG" "$offset_cluster_bits" "\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_cluster_bits" "\x1f"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_cluster_bits" "\x08"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_cluster_bits" "\x11"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
new file mode 100644
index 0000000..8bf8158
--- /dev/null
+++ b/tests/qemu-iotests/092.out
@@ -0,0 +1,13 @@
+QA output created by 092
+
+== Invalid cluster size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+no file open, try 'help open'
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 2988cfd..0f07440 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -98,3 +98,4 @@
 089 rw auto quick
 090 rw auto quick
 091 rw auto
+092 rw auto quick
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/5] qcow1: Validate L2 table size (CVE-2014-0222)

[Kevin Wolf]

Too large L2 table sizes cause unbounded allocations. Images actually
created by qemu-img only have 512 byte or 4k L2 tables.

To keep things consistent with cluster sizes, allow ranges between 512
bytes and 64k (in fact, down to 1 entry = 8 bytes is technically
working, but L2 table sizes smaller than a cluster don't make a lot of
sense).

This also means that the number of bytes on the virtual disk that are
described by the same L2 table is limited to at most 8k * 64k or 2^29,
preventively avoiding any integer overflows.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c               |  8 ++++++++
 tests/qemu-iotests/092     | 15 +++++++++++++++
 tests/qemu-iotests/092.out | 11 +++++++++++
 3 files changed, 34 insertions(+)

diff --git a/block/qcow.c b/block/qcow.c
index e60df23..e8038e5 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -139,6 +139,14 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* l2_bits specifies number of entries; storing a uint64_t in each entry,
+     * so bytes = num_entries << 3. */
+    if (header.l2_bits < 9 - 3 || header.l2_bits > 16 - 3) {
+        error_setg(errp, "L2 table size must be between 512 and 64k");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     if (header.crypt_method > QCOW_CRYPT_AES) {
         error_setg(errp, "invalid encryption method in qcow header");
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
index d060e6f..fb8bacc 100755
--- a/tests/qemu-iotests/092
+++ b/tests/qemu-iotests/092
@@ -44,6 +44,7 @@ _supported_proto generic
 _supported_os Linux
 
 offset_cluster_bits=32
+offset_l2_bits=33
 
 echo
 echo "== Invalid cluster size =="
@@ -57,6 +58,20 @@ poke_file "$TEST_IMG" "$offset_cluster_bits" "\x08"
 poke_file "$TEST_IMG" "$offset_cluster_bits" "\x11"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Invalid L2 table size =="
+_make_test_img 64M
+poke_file "$TEST_IMG" "$offset_l2_bits" "\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_l2_bits" "\x05"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
+# 1 << 0x1b = 2^31 / L2_CACHE_SIZE
+poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
+{ $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/092.out b/tests/qemu-iotests/092.out
index 8bf8158..73918b3 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -10,4 +10,15 @@ qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and
 no file open, try 'help open'
 qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
 no file open, try 'help open'
+
+== Invalid L2 table size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+no file open, try 'help open'
 *** done
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/5] qcow1: Validate image size (CVE-2014-0223)

[Kevin Wolf]

A huge image size could cause s->l1_size to overflow. Make sure that
images never require a L1 table larger than what fits in s->l1_size.

This cannot only cause unbounded allocations, but also the allocation of
a too small L1 table, resulting in out-of-bounds array accesses (both
reads and writes).

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c               | 16 ++++++++++++++--
 tests/qemu-iotests/092     |  9 +++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index e8038e5..3566c05 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -61,7 +61,7 @@ typedef struct BDRVQcowState {
     int cluster_sectors;
     int l2_bits;
     int l2_size;
-    int l1_size;
+    unsigned int l1_size;
     uint64_t cluster_offset_mask;
     uint64_t l1_table_offset;
     uint64_t *l1_table;
@@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* read the level 1 table */
     shift = s->cluster_bits + s->l2_bits;
-    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
+    if (header.size > UINT64_MAX - (1LL << shift)) {
+        error_setg(errp, "Image too large");
+        ret = -EINVAL;
+        goto fail;
+    } else {
+        uint64_t l1_size = (header.size + (1LL << shift) - 1) >> shift;
+        if (l1_size > INT_MAX / sizeof(uint64_t)) {
+            error_setg(errp, "Image too large");
+            ret = -EINVAL;
+            goto fail;
+        }
+        s->l1_size = l1_size;
+    }
 
     s->l1_table_offset = header.l1_table_offset;
     s->l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
index fb8bacc..ae6ca76 100755
--- a/tests/qemu-iotests/092
+++ b/tests/qemu-iotests/092
@@ -43,6 +43,7 @@ _supported_fmt qcow
 _supported_proto generic
 _supported_os Linux
 
+offset_size=24
 offset_cluster_bits=32
 offset_l2_bits=33
 
@@ -72,6 +73,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
 poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
 { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Invalid size =="
+_make_test_img 64M
+poke_file "$TEST_IMG" "$offset_size" "\xee\xee\xee\xee\xee\xee\xee\xee"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_size" "\x7f\xff\xff\xff\xff\xff\xff\xff"
+{ $QEMU_IO -c "write 0 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
index 73918b3..ac03302 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -21,4 +21,11 @@ qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 an
 no file open, try 'help open'
 qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
 no file open, try 'help open'
+
+== Invalid size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+no file open, try 'help open'
 *** done
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/5] qcow1: Stricter backing file length check

[Kevin Wolf]

Like qcow2 since commit 6d33e8e7, error out on invalid lengths instead
of silently truncating them to 1023.

Also don't rely on bdrv_pread() catching integer overflows that make len
negative, but use unsigned variables in the first place.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block/qcow.c               |  7 +++++--
 tests/qemu-iotests/092     | 11 +++++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 3566c05..7fd57d7 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -97,7 +97,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
-    int len, i, shift, ret;
+    unsigned int len, i, shift;
+    int ret;
     QCowHeader header;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
@@ -202,7 +203,9 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
         if (len > 1023) {
-            len = 1023;
+            error_setg(errp, "Backing file name too long");
+            ret = -EINVAL;
+            goto fail;
         }
         ret = bdrv_pread(bs->file, header.backing_file_offset,
                    bs->backing_file, len);
diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
index ae6ca76..a8c0c9c 100755
--- a/tests/qemu-iotests/092
+++ b/tests/qemu-iotests/092
@@ -43,6 +43,8 @@ _supported_fmt qcow
 _supported_proto generic
 _supported_os Linux
 
+offset_backing_file_offset=8
+offset_backing_file_size=16
 offset_size=24
 offset_cluster_bits=32
 offset_l2_bits=33
@@ -81,6 +83,15 @@ poke_file "$TEST_IMG" "$offset_size" "\xee\xee\xee\xee\xee\xee\xee\xee"
 poke_file "$TEST_IMG" "$offset_size" "\x7f\xff\xff\xff\xff\xff\xff\xff"
 { $QEMU_IO -c "write 0 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "== Invalid backing file length =="
+_make_test_img 64M
+poke_file "$TEST_IMG" "$offset_backing_file_offset" "\x00\x00\x00\xff"
+poke_file "$TEST_IMG" "$offset_backing_file_size" "\xff\xff\xff\xff"
+{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+poke_file "$TEST_IMG" "$offset_backing_file_size" "\x7f\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/092.out b/tests/qemu-iotests/092.out
index ac03302..496d8f0 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -28,4 +28,11 @@ qemu-io: can't open device TEST_DIR/t.qcow: Image too large
 no file open, try 'help open'
 qemu-io: can't open device TEST_DIR/t.qcow: Image too large
 no file open, try 'help open'
+
+== Invalid backing file length ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+qemu-io: can't open device TEST_DIR/t.qcow: Backing file name too long
+no file open, try 'help open'
+qemu-io: can't open device TEST_DIR/t.qcow: Backing file name too long
+no file open, try 'help open'
 *** done
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/5] qcow1: Make padding in the header explicit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On 05/15/2014 08:21 AM, Kevin Wolf wrote:
> We were relying on all compilers inserting the same padding in the
> header struct that is used for the on-disk format. Let's not do that.
> Mark the struct as packed and insert an explicit padding field for
> compatibility.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/qcow.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 937dd6d..3684794 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -48,9 +48,10 @@ typedef struct QCowHeader {
>      uint64_t size; /* in bytes */
>      uint8_t cluster_bits;
>      uint8_t l2_bits;
> +    uint16_t padding;
>      uint32_t crypt_method;
>      uint64_t l1_table_offset;
> -} QCowHeader;
> +} QEMU_PACKED QCowHeader;

Is it worth a compile-time assertion that the correct size is achieved?

[I don't know if glib provides such a macro, but gnulib has a verify()
macro that could be used as:

verify(sizeof(QCowHeader) == NNN)

which expands to _Static_assert(sizeof(QCowHeader) == NNN) in new enough
C compilers, and to something like

extern int (*dummy1(void)) [sizeof (struct dummy2 {
  int dummy3: (sizeof(QCowHeader) == NNN) ? 1 : -1; })]

on older compilers for reliable compile-time detection]

But not a show-stopper to this patch as-is.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/5] qcow1: Validate L2 table size (CVE-2014-0222)

[Benoît Canet]

The Thursday 15 May 2014 à 16:21:55 (+0200), Kevin Wolf wrote :
> Too large L2 table sizes cause unbounded allocations. Images actually
> created by qemu-img only have 512 byte or 4k L2 tables.
> 
> To keep things consistent with cluster sizes, allow ranges between 512
> bytes and 64k (in fact, down to 1 entry = 8 bytes is technically
> working, but L2 table sizes smaller than a cluster don't make a lot of
> sense).
> 
> This also means that the number of bytes on the virtual disk that are
> described by the same L2 table is limited to at most 8k * 64k or 2^29,
> preventively avoiding any integer overflows.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c               |  8 ++++++++
>  tests/qemu-iotests/092     | 15 +++++++++++++++
>  tests/qemu-iotests/092.out | 11 +++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index e60df23..e8038e5 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -139,6 +139,14 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    /* l2_bits specifies number of entries; storing a uint64_t in each entry,
> +     * so bytes = num_entries << 3. */
> +    if (header.l2_bits < 9 - 3 || header.l2_bits > 16 - 3) {
> +        error_setg(errp, "L2 table size must be between 512 and 64k");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      if (header.crypt_method > QCOW_CRYPT_AES) {
>          error_setg(errp, "invalid encryption method in qcow header");
>          ret = -EINVAL;
> diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
> index d060e6f..fb8bacc 100755
> --- a/tests/qemu-iotests/092
> +++ b/tests/qemu-iotests/092
> @@ -44,6 +44,7 @@ _supported_proto generic
>  _supported_os Linux
>  
>  offset_cluster_bits=32
> +offset_l2_bits=33
>  
>  echo
>  echo "== Invalid cluster size =="
> @@ -57,6 +58,20 @@ poke_file "$TEST_IMG" "$offset_cluster_bits" "\x08"
>  poke_file "$TEST_IMG" "$offset_cluster_bits" "\x11"
>  { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
>  
> +echo
> +echo "== Invalid L2 table size =="
> +_make_test_img 64M
> +poke_file "$TEST_IMG" "$offset_l2_bits" "\xff"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_l2_bits" "\x05"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +
> +# 1 << 0x1b = 2^31 / L2_CACHE_SIZE
> +poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
> +{ $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/092.out b/tests/qemu-iotests/092.out
> index 8bf8158..73918b3 100644
> --- a/tests/qemu-iotests/092.out
> +++ b/tests/qemu-iotests/092.out
> @@ -10,4 +10,15 @@ qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and
>  no file open, try 'help open'
>  qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
>  no file open, try 'help open'
> +
> +== Invalid L2 table size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> +qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> +no file open, try 'help open'
>  *** done
> -- 
> 1.8.3.1
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v2 2/5] qcow1: Check maximum cluster size

[Benoît Canet]

The Thursday 15 May 2014 à 16:21:54 (+0200), Kevin Wolf wrote :
> Huge values for header.cluster_bits cause unbounded allocations (e.g.
> for s->cluster_cache) and crash qemu this way. Less huge values may
> survive those allocations, but can cause integer overflows later on.
> 
> The only cluster sizes that qemu can create are 4k (for standalone
> images) and 512 (for images with backing files), so we can limit it
> to 64k.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c               | 10 ++++++--
>  tests/qemu-iotests/092     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/092.out | 13 ++++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 85 insertions(+), 2 deletions(-)
>  create mode 100755 tests/qemu-iotests/092
>  create mode 100644 tests/qemu-iotests/092.out
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 3684794..e60df23 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -128,11 +128,17 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    if (header.size <= 1 || header.cluster_bits < 9) {
> -        error_setg(errp, "invalid value in qcow header");
> +    if (header.size <= 1) {
> +        error_setg(errp, "Image size is too small (must be at least 2 bytes)");
>          ret = -EINVAL;
>          goto fail;
>      }
> +    if (header.cluster_bits < 9 || header.cluster_bits > 16) {
> +        error_setg(errp, "Cluster size must be between 512 and 64k");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      if (header.crypt_method > QCOW_CRYPT_AES) {
>          error_setg(errp, "invalid encryption method in qcow header");
>          ret = -EINVAL;
> diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
> new file mode 100755
> index 0000000..d060e6f
> --- /dev/null
> +++ b/tests/qemu-iotests/092
> @@ -0,0 +1,63 @@
> +#!/bin/bash
> +#
> +# qcow1 format input validation tests
> +#
> +# Copyright (C) 2014 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=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    rm -f $TEST_IMG.snap
> +    _cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow
> +_supported_proto generic
> +_supported_os Linux
> +
> +offset_cluster_bits=32
> +
> +echo
> +echo "== Invalid cluster size =="
> +_make_test_img 64M
> +poke_file "$TEST_IMG" "$offset_cluster_bits" "\xff"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_cluster_bits" "\x1f"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_cluster_bits" "\x08"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_cluster_bits" "\x11"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
> new file mode 100644
> index 0000000..8bf8158
> --- /dev/null
> +++ b/tests/qemu-iotests/092.out
> @@ -0,0 +1,13 @@
> +QA output created by 092
> +
> +== Invalid cluster size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> +qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
> +no file open, try 'help open'
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 2988cfd..0f07440 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -98,3 +98,4 @@
>  089 rw auto quick
>  090 rw auto quick
>  091 rw auto
> +092 rw auto quick
> -- 
> 1.8.3.1
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v2 4/5] qcow1: Validate image size (CVE-2014-0223)

[Benoît Canet]

The Thursday 15 May 2014 à 16:21:56 (+0200), Kevin Wolf wrote :
> A huge image size could cause s->l1_size to overflow. Make sure that
> images never require a L1 table larger than what fits in s->l1_size.
> 
> This cannot only cause unbounded allocations, but also the allocation of
> a too small L1 table, resulting in out-of-bounds array accesses (both
> reads and writes).
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c               | 16 ++++++++++++++--
>  tests/qemu-iotests/092     |  9 +++++++++
>  tests/qemu-iotests/092.out |  7 +++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index e8038e5..3566c05 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -61,7 +61,7 @@ typedef struct BDRVQcowState {
>      int cluster_sectors;
>      int l2_bits;
>      int l2_size;
> -    int l1_size;
> +    unsigned int l1_size;
>      uint64_t cluster_offset_mask;
>      uint64_t l1_table_offset;
>      uint64_t *l1_table;
> @@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      /* read the level 1 table */
>      shift = s->cluster_bits + s->l2_bits;
> -    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
> +    if (header.size > UINT64_MAX - (1LL << shift)) {
> +        error_setg(errp, "Image too large");
> +        ret = -EINVAL;
> +        goto fail;
> +    } else {
> +        uint64_t l1_size = (header.size + (1LL << shift) - 1) >> shift;
> +        if (l1_size > INT_MAX / sizeof(uint64_t)) {
> +            error_setg(errp, "Image too large");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +        s->l1_size = l1_size;
> +    }
>  
>      s->l1_table_offset = header.l1_table_offset;
>      s->l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
> diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
> index fb8bacc..ae6ca76 100755
> --- a/tests/qemu-iotests/092
> +++ b/tests/qemu-iotests/092
> @@ -43,6 +43,7 @@ _supported_fmt qcow
>  _supported_proto generic
>  _supported_os Linux
>  
> +offset_size=24
>  offset_cluster_bits=32
>  offset_l2_bits=33
>  
> @@ -72,6 +73,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
>  poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
>  { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
>  
> +echo
> +echo "== Invalid size =="
> +_make_test_img 64M
> +poke_file "$TEST_IMG" "$offset_size" "\xee\xee\xee\xee\xee\xee\xee\xee"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_size" "\x7f\xff\xff\xff\xff\xff\xff\xff"
> +{ $QEMU_IO -c "write 0 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
> index 73918b3..ac03302 100644
> --- a/tests/qemu-iotests/092.out
> +++ b/tests/qemu-iotests/092.out
> @@ -21,4 +21,11 @@ qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 an
>  no file open, try 'help open'
>  qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
>  no file open, try 'help open'
> +
> +== Invalid size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> +no file open, try 'help open'
>  *** done
> -- 
> 1.8.3.1
> 

I still not understand this one :)

Re: [Qemu-devel] [PATCH v2 1/5] qcow1: Make padding in the header explicit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

Am 15.05.2014 um 17:49 hat Eric Blake geschrieben:
> On 05/15/2014 08:21 AM, Kevin Wolf wrote:
> > We were relying on all compilers inserting the same padding in the
> > header struct that is used for the on-disk format. Let's not do that.
> > Mark the struct as packed and insert an explicit padding field for
> > compatibility.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block/qcow.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/qcow.c b/block/qcow.c
> > index 937dd6d..3684794 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> > @@ -48,9 +48,10 @@ typedef struct QCowHeader {
> >      uint64_t size; /* in bytes */
> >      uint8_t cluster_bits;
> >      uint8_t l2_bits;
> > +    uint16_t padding;
> >      uint32_t crypt_method;
> >      uint64_t l1_table_offset;
> > -} QCowHeader;
> > +} QEMU_PACKED QCowHeader;
> 
> Is it worth a compile-time assertion that the correct size is achieved?
> 
> [I don't know if glib provides such a macro, but gnulib has a verify()
> macro that could be used as:
> 
> verify(sizeof(QCowHeader) == NNN)
> 
> which expands to _Static_assert(sizeof(QCowHeader) == NNN) in new enough
> C compilers, and to something like
> 
> extern int (*dummy1(void)) [sizeof (struct dummy2 {
>   int dummy3: (sizeof(QCowHeader) == NNN) ? 1 : -1; })]
> 
> on older compilers for reliable compile-time detection]
> 
> But not a show-stopper to this patch as-is.

QEMU_BUILD_BUG_ON() is what you're looking for.

Do you think that would be a useful addition? With packed structs there
should be little that could make it go wrong. But if we want to add
this, I'd do it in a separate patch and for all image formats.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 4/5] qcow1: Validate image size (CVE-2014-0223)

[Kevin Wolf]

Am 15.05.2014 um 18:35 hat Benoît Canet geschrieben:
> The Thursday 15 May 2014 à 16:21:56 (+0200), Kevin Wolf wrote :
> > A huge image size could cause s->l1_size to overflow. Make sure that
> > images never require a L1 table larger than what fits in s->l1_size.
> > 
> > This cannot only cause unbounded allocations, but also the allocation of
> > a too small L1 table, resulting in out-of-bounds array accesses (both
> > reads and writes).
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow.c               | 16 ++++++++++++++--
> >  tests/qemu-iotests/092     |  9 +++++++++
> >  tests/qemu-iotests/092.out |  7 +++++++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow.c b/block/qcow.c
> > index e8038e5..3566c05 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> > @@ -61,7 +61,7 @@ typedef struct BDRVQcowState {
> >      int cluster_sectors;
> >      int l2_bits;
> >      int l2_size;
> > -    int l1_size;
> > +    unsigned int l1_size;
> >      uint64_t cluster_offset_mask;
> >      uint64_t l1_table_offset;
> >      uint64_t *l1_table;
> > @@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      /* read the level 1 table */
> >      shift = s->cluster_bits + s->l2_bits;
> > -    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > +    if (header.size > UINT64_MAX - (1LL << shift)) {
> > +        error_setg(errp, "Image too large");
> > +        ret = -EINVAL;
> > +        goto fail;
> > +    } else {
> > +        uint64_t l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > +        if (l1_size > INT_MAX / sizeof(uint64_t)) {
> > +            error_setg(errp, "Image too large");
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +        s->l1_size = l1_size;
> > +    }
> >  
> >      s->l1_table_offset = header.l1_table_offset;
> >      s->l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
> > diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
> > index fb8bacc..ae6ca76 100755
> > --- a/tests/qemu-iotests/092
> > +++ b/tests/qemu-iotests/092
> > @@ -43,6 +43,7 @@ _supported_fmt qcow
> >  _supported_proto generic
> >  _supported_os Linux
> >  
> > +offset_size=24
> >  offset_cluster_bits=32
> >  offset_l2_bits=33
> >  
> > @@ -72,6 +73,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
> >  poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
> >  { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> >  
> > +echo
> > +echo "== Invalid size =="
> > +_make_test_img 64M
> > +poke_file "$TEST_IMG" "$offset_size" "\xee\xee\xee\xee\xee\xee\xee\xee"
> > +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> > +poke_file "$TEST_IMG" "$offset_size" "\x7f\xff\xff\xff\xff\xff\xff\xff"
> > +{ $QEMU_IO -c "write 0 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> > +
> >  # success, all done
> >  echo "*** done"
> >  rm -f $seq.full
> > diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
> > index 73918b3..ac03302 100644
> > --- a/tests/qemu-iotests/092.out
> > +++ b/tests/qemu-iotests/092.out
> > @@ -21,4 +21,11 @@ qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 an
> >  no file open, try 'help open'
> >  qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> >  no file open, try 'help open'
> > +
> > +== Invalid size ==
> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> > +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> > +no file open, try 'help open'
> > +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> > +no file open, try 'help open'
> >  *** done
> > -- 
> > 1.8.3.1
> > 
> 
> I still not understand this one :)

I know, and I'm sorry about that, but I can't explain better than I did.

Last attempt: There is this code line in the original source:

    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;

There are two different integer overflows that can happen in this line.
header.size + (1LL << shift) could exceed UINT64_MAX (header.size is
uint64_t), and the whole calculation could exceed INT_MAX (s->l1_size is
int).

My patch checks for these two conditions and leaves everything else as
it is. It has nothing to do with any L2 table sizes or anything.

Kevin

Re: [Qemu-devel] [PATCH v2 4/5] qcow1: Validate image size (CVE-2014-0223)

[Benoît Canet]

The Friday 16 May 2014 à 12:47:56 (+0200), Kevin Wolf wrote :
> Am 15.05.2014 um 18:35 hat Benoît Canet geschrieben:
> > The Thursday 15 May 2014 à 16:21:56 (+0200), Kevin Wolf wrote :
> > > A huge image size could cause s->l1_size to overflow. Make sure that
> > > images never require a L1 table larger than what fits in s->l1_size.
> > > 
> > > This cannot only cause unbounded allocations, but also the allocation of
> > > a too small L1 table, resulting in out-of-bounds array accesses (both
> > > reads and writes).
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/qcow.c               | 16 ++++++++++++++--
> > >  tests/qemu-iotests/092     |  9 +++++++++
> > >  tests/qemu-iotests/092.out |  7 +++++++
> > >  3 files changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/qcow.c b/block/qcow.c
> > > index e8038e5..3566c05 100644
> > > --- a/block/qcow.c
> > > +++ b/block/qcow.c
> > > @@ -61,7 +61,7 @@ typedef struct BDRVQcowState {
> > >      int cluster_sectors;
> > >      int l2_bits;
> > >      int l2_size;
> > > -    int l1_size;
> > > +    unsigned int l1_size;
> > >      uint64_t cluster_offset_mask;
> > >      uint64_t l1_table_offset;
> > >      uint64_t *l1_table;
> > > @@ -166,7 +166,19 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> > >  
> > >      /* read the level 1 table */
> > >      shift = s->cluster_bits + s->l2_bits;
> > > -    s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > > +    if (header.size > UINT64_MAX - (1LL << shift)) {
> > > +        error_setg(errp, "Image too large");
> > > +        ret = -EINVAL;
> > > +        goto fail;
> > > +    } else {
> > > +        uint64_t l1_size = (header.size + (1LL << shift) - 1) >> shift;
> > > +        if (l1_size > INT_MAX / sizeof(uint64_t)) {
> > > +            error_setg(errp, "Image too large");
> > > +            ret = -EINVAL;
> > > +            goto fail;
> > > +        }
> > > +        s->l1_size = l1_size;
> > > +    }
> > >  
> > >      s->l1_table_offset = header.l1_table_offset;
> > >      s->l1_table = g_malloc(s->l1_size * sizeof(uint64_t));
> > > diff --git a/tests/qemu-iotests/092 b/tests/qemu-iotests/092
> > > index fb8bacc..ae6ca76 100755
> > > --- a/tests/qemu-iotests/092
> > > +++ b/tests/qemu-iotests/092
> > > @@ -43,6 +43,7 @@ _supported_fmt qcow
> > >  _supported_proto generic
> > >  _supported_os Linux
> > >  
> > > +offset_size=24
> > >  offset_cluster_bits=32
> > >  offset_l2_bits=33
> > >  
> > > @@ -72,6 +73,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\x0e"
> > >  poke_file "$TEST_IMG" "$offset_l2_bits" "\x1b"
> > >  { $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> > >  
> > > +echo
> > > +echo "== Invalid size =="
> > > +_make_test_img 64M
> > > +poke_file "$TEST_IMG" "$offset_size" "\xee\xee\xee\xee\xee\xee\xee\xee"
> > > +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> > > +poke_file "$TEST_IMG" "$offset_size" "\x7f\xff\xff\xff\xff\xff\xff\xff"
> > > +{ $QEMU_IO -c "write 0 64M" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> > > +
> > >  # success, all done
> > >  echo "*** done"
> > >  rm -f $seq.full
> > > diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
> > > index 73918b3..ac03302 100644
> > > --- a/tests/qemu-iotests/092.out
> > > +++ b/tests/qemu-iotests/092.out
> > > @@ -21,4 +21,11 @@ qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 an
> > >  no file open, try 'help open'
> > >  qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
> > >  no file open, try 'help open'
> > > +
> > > +== Invalid size ==
> > > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> > > +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> > > +no file open, try 'help open'
> > > +qemu-io: can't open device TEST_DIR/t.qcow: Image too large
> > > +no file open, try 'help open'
> > >  *** done
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > I still not understand this one :)
> 
> I know, and I'm sorry about that, but I can't explain better than I did.
> 
> Last attempt: There is this code line in the original source:
> 
>     s->l1_size = (header.size + (1LL << shift) - 1) >> shift;
> 
> There are two different integer overflows that can happen in this line.
> header.size + (1LL << shift) could exceed UINT64_MAX (header.size is
> uint64_t), and the whole calculation could exceed INT_MAX (s->l1_size is
> int).
> 
> My patch checks for these two conditions and leaves everything else as
> it is. It has nothing to do with any L2 table sizes or anything.

I understand now.
Thanks.

Best regards

Benoît
> 
> Kevin

Re: [Qemu-devel] [PATCH v2 1/5] qcow1: Make padding in the header explicit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On 05/16/2014 04:41 AM, Kevin Wolf wrote:

>>> +} QEMU_PACKED QCowHeader;
>>
>> Is it worth a compile-time assertion that the correct size is achieved?
>>

> 
> QEMU_BUILD_BUG_ON() is what you're looking for.

Ah, thanks.

> 
> Do you think that would be a useful addition? With packed structs there
> should be little that could make it go wrong. But if we want to add
> this, I'd do it in a separate patch and for all image formats.

Definitely a separate patch.  But yes, I think that it can't hurt - even
with packed structs, it's a bit of insurance against someone
accidentally adding a field or using a wrong type, particularly for any
packed structs where a newer version converts reserved space into a
named purpose.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]