qcow1: Input validation fixes

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

---
 block/qcow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/qcow.c +0 -0

index 937dd6d..3684794 100644

Quoted 17 lines

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

---
 block/qcow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/qcow.c +0 -0

index 937dd6d..3684794 100644

block/qcow.c typedef struct QCowHeader { · line 54

Quoted 8 lines

block/qcow.c +2 -1

@@ -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.

Quoted 4 lines

Quoted 1 line

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

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.

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

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

block/qcow.c +8 -2

index 3684794..e60df23 100644
@@ -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;

tests/qemu-iotests/092 new file +63 -0

new file mode 100755
index 0000000..d060e6f
@@ -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
[email protected]
+
+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

tests/qemu-iotests/092.out new file +13 -0

new file mode 100644
index 0000000..8bf8158
@@ -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

tests/qemu-iotests/group +1 -0

index 2988cfd..0f07440 100644
@@ -98,3 +98,4 @@
 089 rw auto quick
 090 rw auto quick
 091 rw auto
+092 rw auto quick

Signature · 1 line

1.8.3.1

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow.c               |  8 ++++++++
 tests/qemu-iotests/092     | 15 +++++++++++++++
 tests/qemu-iotests/092.out | 11 +++++++++++
 3 files changed, 34 insertions(+)

block/qcow.c +8 -0

index e60df23..e8038e5 100644
@@ -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;

tests/qemu-iotests/092 +15 -0

index d060e6f..fb8bacc 100755
@@ -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

tests/qemu-iotests/092.out +11 -0

index 8bf8158..73918b3 100644
@@ -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

Signature · 1 line

1.8.3.1

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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow.c               | 16 ++++++++++++++--
 tests/qemu-iotests/092     |  9 +++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

block/qcow.c +14 -2

index e8038e5..3566c05 100644
@@ -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));

tests/qemu-iotests/092 +9 -0

index fb8bacc..ae6ca76 100755
@@ -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

tests/qemu-iotests/092.out +7 -0

index 73918b3..ac03302 100644
@@ -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

Signature · 1 line

1.8.3.1

Quoted 89 lines · 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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow.c               | 16 ++++++++++++++--
 tests/qemu-iotests/092     |  9 +++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

block/qcow.c +14 -2

index e8038e5..3566c05 100644
@@ -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));

tests/qemu-iotests/092 +9 -0

index fb8bacc..ae6ca76 100755
@@ -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

tests/qemu-iotests/092.out +5 -0

index 73918b3..ac03302 100644
@@ -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'

Quoted 6 lines

tests/qemu-iotests/092.out +2 -0

+qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+no file open, try 'help open'
 *** done

Signature · 1 line

1.8.3.1

I still not understand this one :)

Am 15.05.2014 um 18:35 hat Benoît Canet geschrieben:

Quoted 90 lines

Quoted 89 lines · 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 [email protected]

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow.c               | 16 ++++++++++++++--
 tests/qemu-iotests/092     |  9 +++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

block/qcow.c +14 -2

index e8038e5..3566c05 100644
@@ -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));

tests/qemu-iotests/092 +9 -0

index fb8bacc..ae6ca76 100755
@@ -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

tests/qemu-iotests/092.out +5 -0

index 73918b3..ac03302 100644
@@ -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'

Quoted 8 lines

Quoted 6 lines

tests/qemu-iotests/092.out +2 -0

+qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+no file open, try 'help open'
 *** done

Signature · 1 line

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