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

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

[Kevin Wolf]

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     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/092.out | 30 ++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 158 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 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 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 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c               | 10 ++++++--
 tests/qemu-iotests/092     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/092.out |  9 +++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 78 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..b0f04e3
--- /dev/null
+++ b/tests/qemu-iotests/092
@@ -0,0 +1,60 @@
+#!/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
+offset_l2_bits=33
+
+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
+
+# 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..9e7367a
--- /dev/null
+++ b/tests/qemu-iotests/092.out
@@ -0,0 +1,9 @@
+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'
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cd3e4d2..ca39ab3 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -97,3 +97,4 @@
 088 rw auto
 090 rw auto quick
 091 rw auto
+092 rw auto quick
-- 
1.8.3.1

[Qemu-devel] [PATCH 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c               |  8 ++++++++
 tests/qemu-iotests/092     | 10 ++++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 25 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 b0f04e3..2196cce 100755
--- a/tests/qemu-iotests/092
+++ b/tests/qemu-iotests/092
@@ -54,6 +54,16 @@ poke_file "$TEST_IMG" "$offset_cluster_bits" "\xff"
 poke_file "$TEST_IMG" "$offset_cluster_bits" "\x1f"
 { $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
+
+# 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 9e7367a..45a7ac8 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -6,4 +6,11 @@ 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'
 *** done
-- 
1.8.3.1

[Qemu-devel] [PATCH 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).

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 2196cce..26a1324 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
 
@@ -64,6 +65,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\xff"
 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 45a7ac8..c3678a0 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -13,4 +13,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 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 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 26a1324..b1333a0 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
@@ -73,6 +75,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 c3678a0..e957887 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -20,4 +20,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 1/5] qcow1: Make padding in the header explicit

[Benoît Canet]

The Monday 12 May 2014 à 15:04:07 (+0200), 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Monday 12 May 2014 à 15:04:08 (+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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c               | 10 ++++++--
>  tests/qemu-iotests/092     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/092.out |  9 +++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 78 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..b0f04e3
> --- /dev/null
> +++ b/tests/qemu-iotests/092
> @@ -0,0 +1,60 @@
> +#!/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

> +offset_l2_bits=33
This seems to be an extra.

> +
> +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

>From the code " +    if (header.cluster_bits < 9 || header.cluster_bits > 16) {"

Shouldn't the hex values be "\x08" and "\x11" ?

> +
> +# 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..9e7367a
> --- /dev/null
> +++ b/tests/qemu-iotests/092.out
> @@ -0,0 +1,9 @@
> +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'
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index cd3e4d2..ca39ab3 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -97,3 +97,4 @@
>  088 rw auto
>  090 rw auto quick
>  091 rw auto
> +092 rw auto quick
> -- 
> 1.8.3.1
> 
> 

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

[Benoît Canet]

The Monday 12 May 2014 à 15:04:09 (+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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c               |  8 ++++++++
>  tests/qemu-iotests/092     | 10 ++++++++++
>  tests/qemu-iotests/092.out |  7 +++++++
>  3 files changed, 25 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 b0f04e3..2196cce 100755
> --- a/tests/qemu-iotests/092
> +++ b/tests/qemu-iotests/092
> @@ -54,6 +54,16 @@ poke_file "$TEST_IMG" "$offset_cluster_bits" "\xff"
>  poke_file "$TEST_IMG" "$offset_cluster_bits" "\x1f"
>  { $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

I see why $offset_l2_bit was present in the previous patch now.

> +poke_file "$TEST_IMG" "$offset_l2_bits" "\xff"
> +{ $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

The lower limit is not tested "\x05".
Some extra room is present for testing the high limit "\0x0e"

> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
> index 9e7367a..45a7ac8 100644
> --- a/tests/qemu-iotests/092.out
> +++ b/tests/qemu-iotests/092.out
> @@ -6,4 +6,11 @@ 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'
>  *** done
> -- 
> 1.8.3.1
> 
> 

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

[Benoît Canet]

The Monday 12 May 2014 à 15:04:10 (+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).
> 
> 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)) {

I won't be much helpfull but this feel wrong.
Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cluster_bits + s->l2_bits) bytes ?
Where the size for the L2 chunk themselves is accounted ?

> +        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 2196cce..26a1324 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
>  
> @@ -64,6 +65,14 @@ poke_file "$TEST_IMG" "$offset_l2_bits" "\xff"
>  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 45a7ac8..c3678a0 100644
> --- a/tests/qemu-iotests/092.out
> +++ b/tests/qemu-iotests/092.out
> @@ -13,4 +13,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
> 
> 

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

[Benoît Canet]

The Monday 12 May 2014 à 15:04:11 (+0200), Kevin Wolf wrote :
> 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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 26a1324..b1333a0 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
> @@ -73,6 +75,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 c3678a0..e957887 100644
> --- a/tests/qemu-iotests/092.out
> +++ b/tests/qemu-iotests/092.out
> @@ -20,4 +20,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
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Kevin Wolf]

Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:
> The Monday 12 May 2014 à 15:04:10 (+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).
> > 
> > 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)) {
> 
> I won't be much helpfull but this feel wrong.
> Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cluster_bits + s->l2_bits) bytes ?
> Where the size for the L2 chunk themselves is accounted ?

Not sure what your concern is, but this is basically the same system as
with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map
virtual disk clusters to image file clusters. They don't map metadata
like themselves.

One cluster contains (1 << cluster_bits) bytes. One L2 table contains
mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits +
l2_bits)) is the number of bytes on the virtual disk that are described
by a single L2 table.

All of this is not related to this patch. All I'm doing here is catching
integer overflows in the calculation of s->l1_size. Apart from error
cases, the calculation is unchanged.

Kevin

> > +        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));

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

[Benoît Canet]

The Monday 12 May 2014 à 18:43:33 (+0200), Kevin Wolf wrote :
> Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:
> > The Monday 12 May 2014 à 15:04:10 (+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).
> > > 
> > > 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)) {
> > 
> > I won't be much helpfull but this feel wrong.
> > Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cluster_bits + s->l2_bits) bytes ?
> > Where the size for the L2 chunk themselves is accounted ?
> 
> Not sure what your concern is, but this is basically the same system as
> with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map
> virtual disk clusters to image file clusters. They don't map metadata
> like themselves.
> 
> One cluster contains (1 << cluster_bits) bytes. One L2 table contains
> mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits +
> l2_bits)) is the number of bytes on the virtual disk that are described
> by a single L2 table.

I am under the impression that this test compute the maximum size left for
the header.

So as there is probably more that one L2 table the space left for the header
is 1 - nb_l2_table * number_of_byte_covered_by_l2 - number of byte of l1 - number of 
bytes of l2 themselve.

> 
> All of this is not related to this patch. All I'm doing here is catching
> integer overflows in the calculation of s->l1_size. Apart from error
> cases, the calculation is unchanged.
> 
> Kevin
> 
> > > +        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));
> 

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

[Benoît Canet]

The Monday 12 May 2014 à 19:04:22 (+0200), Benoît Canet wrote :
> The Monday 12 May 2014 à 18:43:33 (+0200), Kevin Wolf wrote :
> > Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:
> > > The Monday 12 May 2014 à 15:04:10 (+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).
> > > > 
> > > > 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)) {
> > > 
> > > I won't be much helpfull but this feel wrong.
> > > Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cluster_bits + s->l2_bits) bytes ?
> > > Where the size for the L2 chunk themselves is accounted ?
> > 
> > Not sure what your concern is, but this is basically the same system as
> > with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map
> > virtual disk clusters to image file clusters. They don't map metadata
> > like themselves.
> > 
> > One cluster contains (1 << cluster_bits) bytes. One L2 table contains
> > mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits +
> > l2_bits)) is the number of bytes on the virtual disk that are described
> > by a single L2 table.
> 
> I am under the impression that this test compute the maximum size left for
> the header.
> 
> So as there is probably more that one L2 table the space left for the header
> is 1 - nb_l2_table * number_of_byte_covered_by_l2 - number of byte of l1 - number of 
> bytes of l2 themselve.

I got this part wrong but still we must account that there could be multiple l2 tables.

> 
> > 
> > All of this is not related to this patch. All I'm doing here is catching
> > integer overflows in the calculation of s->l1_size. Apart from error
> > cases, the calculation is unchanged.
> > 
> > Kevin
> > 
> > > > +        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));
> > 
> 

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

[Kevin Wolf]

Am 12.05.2014 um 19:04 hat Benoît Canet geschrieben:
> The Monday 12 May 2014 à 18:43:33 (+0200), Kevin Wolf wrote :
> > Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:
> > > The Monday 12 May 2014 à 15:04:10 (+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).
> > > > 
> > > > 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)) {
> > > 
> > > I won't be much helpfull but this feel wrong.
> > > Does each l1 entry point to an l2 chunk mapping itself to 1 << (s->cluster_bits + s->l2_bits) bytes ?
> > > Where the size for the L2 chunk themselves is accounted ?
> > 
> > Not sure what your concern is, but this is basically the same system as
> > with qcow2: L1 entries point to the offsets of L2 tables. L2 tables map
> > virtual disk clusters to image file clusters. They don't map metadata
> > like themselves.
> > 
> > One cluster contains (1 << cluster_bits) bytes. One L2 table contains
> > mappings for (1 << l2_bits) clusters. Therefore, (1 << (cluster_bits +
> > l2_bits)) is the number of bytes on the virtual disk that are described
> > by a single L2 table.
> 
> I am under the impression that this test compute the maximum size left for
> the header.

No, it doesn't. It only ensures that (header.size + (1LL << shift) - 1)
doesn't overflow, which is part of rounding up the image size.

Kevin

> So as there is probably more that one L2 table the space left for the header
> is 1 - nb_l2_table * number_of_byte_covered_by_l2 - number of byte of l1 - number of 
> bytes of l2 themselve.
> 
> > 
> > All of this is not related to this patch. All I'm doing here is catching
> > integer overflows in the calculation of s->l1_size. Apart from error
> > cases, the calculation is unchanged.
> > 
> > Kevin
> > 
> > > > +        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));
> > 

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

[Stefan Hajnoczi]

On Mon, May 12, 2014 at 03:04:06PM +0200, Kevin Wolf wrote:
> 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     | 90 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/092.out | 30 ++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 158 insertions(+), 7 deletions(-)
>  create mode 100755 tests/qemu-iotests/092
>  create mode 100644 tests/qemu-iotests/092.out

Looks good overall.  No Reviewed-by yet because I'm curious about the
answer to Benoit's questions about the poke_file constants.

Stefan

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

[Kevin Wolf]

Am 12.05.2014 um 17:00 hat Benoît Canet geschrieben:
> The Monday 12 May 2014 à 15:04:08 (+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.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow.c               | 10 ++++++--
> >  tests/qemu-iotests/092     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/092.out |  9 +++++++
> >  tests/qemu-iotests/group   |  1 +
> >  4 files changed, 78 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..b0f04e3
> > --- /dev/null
> > +++ b/tests/qemu-iotests/092
> > @@ -0,0 +1,60 @@
> > +#!/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
> 
> > +offset_l2_bits=33
> This seems to be an extra.

Moving this line to the next patch.

> > +
> > +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
> 
> From the code " +    if (header.cluster_bits < 9 || header.cluster_bits > 16) {"
> 
> Shouldn't the hex values be "\x08" and "\x11" ?

Those values actually kind of work, it needs something bigger to crash
qemu. But I'll add those two values as well.

Kevin