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.

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

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

block/qcow.c +2 -1

index 937dd6d..3684794 100644
@@ -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
 

Signature · 1 line

1.8.3.1

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

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

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

new file mode 100755
index 0000000..b0f04e3
@@ -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
[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

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

+
+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) {"

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

+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0

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

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

tests/qemu-iotests/group +1 -0

index cd3e4d2..ca39ab3 100644
@@ -97,3 +97,4 @@
 088 rw auto
 090 rw auto quick
 091 rw auto
+092 rw auto quick

Signature · 1 line

1.8.3.1

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

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

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

new file mode 100755
index 0000000..b0f04e3
@@ -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
[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

tests/qemu-iotests/092 line 47

Quoted 1 line

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

+offset_l2_bits=33

This seems to be an extra.

Quoted 4 lines

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

+
+echo
+echo "== Invalid cluster size =="
+_make_test_img 64M

tests/qemu-iotests/092 line 55

Quoted 4 lines

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

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

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

---
 block/qcow.c               |  8 ++++++++
 tests/qemu-iotests/092     | 10 ++++++++++
 tests/qemu-iotests/092.out |  7 +++++++
 3 files changed, 25 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 +0 -0

index b0f04e3..2196cce 100755

tests/qemu-iotests/092 +1 -0

+
 # success, all done
 echo "*** done"
 rm -f $seq.full

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

index 9e7367a..45a7ac8 100644
@@ -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

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

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

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;

block/qcow.c +12 -0

+        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 2196cce..26a1324 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
 

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

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

index 45a7ac8..c3678a0 100644
@@ -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

Signature · 1 line

1.8.3.1

Quoted 29 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).

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

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,
 

block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 169

Quoted 4 lines

block/qcow.c +1 -1

     /* 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 ?

Quoted 15 lines

block/qcow.c +12 -0

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

Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:

Quoted 30 lines

Quoted 29 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).

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

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,
 

Quoted 8 lines

block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 169

Quoted 4 lines

block/qcow.c +1 -1

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

Quoted 15 lines

Quoted 15 lines

block/qcow.c +12 -0

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

Quoted 45 lines · Kevin Wolf

Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:

Quoted 30 lines

Quoted 29 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).

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

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,
 

Quoted 8 lines

block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 169

Quoted 4 lines

block/qcow.c +1 -1

     /* 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.

Quoted 4 lines

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.

Quoted 24 lines

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

Quoted 15 lines

Quoted 15 lines

block/qcow.c +12 -0

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

Quoted 45 lines · Kevin Wolf

Am 12.05.2014 um 17:50 hat Benoît Canet geschrieben:

Quoted 30 lines

Quoted 29 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).

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

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,
 

Quoted 8 lines

block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 169

Quoted 4 lines

block/qcow.c +1 -1

     /* 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.

Quoted 4 lines

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.

Quoted 23 lines

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

Quoted 15 lines

Quoted 15 lines

block/qcow.c +12 -0

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

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

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

block/qcow.c +5 -2

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

tests/qemu-iotests/092 +11 -0

index 26a1324..b1333a0 100755
@@ -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

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

index c3678a0..e957887 100644
@@ -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

Signature · 1 line

1.8.3.1

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