* [Qemu-devel] [PATCH v2 1/7] qcow2: Generalize validate_table_offset() into qcow2_validate_table()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 2/7] qcow2: Check L1 table offset in qcow2_snapshot_load_tmp() Alberto Garcia
` (6 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
This function checks that the offset and size of a table are valid.
While the offset checks are fine, the size check is too generic, since
it only verifies that the total size in bytes fits in a 64-bit
integer. In practice all tables used in qcow2 have much smaller size
limits, so the size needs to be checked again for each table using its
actual limit.
This patch generalizes this function by allowing the caller to specify
the maximum size for that table. In addition to that it allows passing
an Error variable.
The function is also renamed and made public since we're going to use
it in other parts of the code.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.c | 77 ++++++++++++++++++----------------------------
block/qcow2.h | 10 +++---
tests/qemu-iotests/080.out | 16 +++++-----
3 files changed, 43 insertions(+), 60 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 071dc4d608..1ab6abf3b5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -559,26 +559,23 @@ static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result,
return ret;
}
-static int validate_table_offset(BlockDriverState *bs, uint64_t offset,
- uint64_t entries, size_t entry_len)
+int qcow2_validate_table(BlockDriverState *bs, uint64_t offset,
+ uint64_t entries, size_t entry_len,
+ int64_t max_size_bytes, const char *table_name,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t size;
+
+ if (entries > max_size_bytes / entry_len) {
+ error_setg(errp, "%s too large", table_name);
+ return -EFBIG;
+ }
/* Use signed INT64_MAX as the maximum even for uint64_t header fields,
* because values will be passed to qemu functions taking int64_t. */
- if (entries > INT64_MAX / entry_len) {
- return -EINVAL;
- }
-
- size = entries * entry_len;
-
- if (INT64_MAX - size < offset) {
- return -EINVAL;
- }
-
- /* Tables must be cluster aligned */
- if (offset_into_cluster(s, offset) != 0) {
+ if ((INT64_MAX - entries * entry_len < offset) ||
+ (offset_into_cluster(s, offset) != 0)) {
+ error_setg(errp, "%s offset invalid", table_name);
return -EINVAL;
}
@@ -1308,47 +1305,42 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
s->refcount_table_size =
header.refcount_table_clusters << (s->cluster_bits - 3);
- if (header.refcount_table_clusters > qcow2_max_refcount_clusters(s)) {
- error_setg(errp, "Reference count table too large");
- ret = -EINVAL;
- goto fail;
- }
-
if (header.refcount_table_clusters == 0 && !(flags & BDRV_O_CHECK)) {
error_setg(errp, "Image does not contain a reference count table");
ret = -EINVAL;
goto fail;
}
- ret = validate_table_offset(bs, s->refcount_table_offset,
- s->refcount_table_size, sizeof(uint64_t));
+ ret = qcow2_validate_table(bs, s->refcount_table_offset,
+ header.refcount_table_clusters,
+ s->cluster_size, QCOW_MAX_REFTABLE_SIZE,
+ "Reference count table", errp);
if (ret < 0) {
- error_setg(errp, "Invalid reference count table offset");
goto fail;
}
- /* Snapshot table offset/length */
- if (header.nb_snapshots > QCOW_MAX_SNAPSHOTS) {
- error_setg(errp, "Too many snapshots");
- ret = -EINVAL;
- goto fail;
- }
-
- ret = validate_table_offset(bs, header.snapshots_offset,
- header.nb_snapshots,
- sizeof(QCowSnapshotHeader));
+ /* The total size in bytes of the snapshot table is checked in
+ * qcow2_read_snapshots() because the size of each snapshot is
+ * variable and we don't know it yet.
+ * Here we only check the offset and number of snapshots. */
+ ret = qcow2_validate_table(bs, header.snapshots_offset,
+ header.nb_snapshots,
+ sizeof(QCowSnapshotHeader),
+ sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
+ "Snapshot table", errp);
if (ret < 0) {
- error_setg(errp, "Invalid snapshot table offset");
goto fail;
}
/* read the level 1 table */
- if (header.l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
- error_setg(errp, "Active L1 table too large");
- ret = -EFBIG;
+ ret = qcow2_validate_table(bs, header.l1_table_offset,
+ header.l1_size, sizeof(uint64_t),
+ QCOW_MAX_L1_SIZE, "Active L1 table", errp);
+ if (ret < 0) {
goto fail;
}
s->l1_size = header.l1_size;
+ s->l1_table_offset = header.l1_table_offset;
l1_vm_state_index = size_to_l1(s, header.size);
if (l1_vm_state_index > INT_MAX) {
@@ -1366,15 +1358,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
- ret = validate_table_offset(bs, header.l1_table_offset,
- header.l1_size, sizeof(uint64_t));
- if (ret < 0) {
- error_setg(errp, "Invalid L1 table offset");
- goto fail;
- }
- s->l1_table_offset = header.l1_table_offset;
-
-
if (s->l1_size > 0) {
s->l1_table = qemu_try_blockalign(bs->file->bs,
ROUND_UP(s->l1_size * sizeof(uint64_t), 512));
diff --git a/block/qcow2.h b/block/qcow2.h
index 1a84cc77b0..2d24855d8e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -485,11 +485,6 @@ static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s)
return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits);
}
-static inline uint64_t qcow2_max_refcount_clusters(BDRVQcow2State *s)
-{
- return QCOW_MAX_REFTABLE_SIZE >> s->cluster_bits;
-}
-
static inline QCow2ClusterType qcow2_get_cluster_type(uint64_t l2_entry)
{
if (l2_entry & QCOW_OFLAG_COMPRESSED) {
@@ -547,6 +542,11 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
int64_t size, const char *message_format, ...)
GCC_FMT_ATTR(5, 6);
+int qcow2_validate_table(BlockDriverState *bs, uint64_t offset,
+ uint64_t entries, size_t entry_len,
+ int64_t max_size_bytes, const char *table_name,
+ Error **errp);
+
/* qcow2-refcount.c functions */
int qcow2_refcount_init(BlockDriverState *bs);
void qcow2_refcount_close(BlockDriverState *bs);
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 6a7fda1356..4c7790c9ee 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -18,18 +18,18 @@ can't open device TEST_DIR/t.qcow2: Reference count table too large
== Misaligned refcount table ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
+can't open device TEST_DIR/t.qcow2: Reference count table offset invalid
== Huge refcount offset ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
+can't open device TEST_DIR/t.qcow2: Reference count table offset invalid
== Invalid snapshot table ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-can't open device TEST_DIR/t.qcow2: Too many snapshots
-can't open device TEST_DIR/t.qcow2: Too many snapshots
-can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
-can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
+can't open device TEST_DIR/t.qcow2: Snapshot table too large
+can't open device TEST_DIR/t.qcow2: Snapshot table too large
+can't open device TEST_DIR/t.qcow2: Snapshot table offset invalid
+can't open device TEST_DIR/t.qcow2: Snapshot table offset invalid
== Hitting snapshot table size limit ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -41,8 +41,8 @@ read 512/512 bytes at offset 0
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
can't open device TEST_DIR/t.qcow2: Active L1 table too large
can't open device TEST_DIR/t.qcow2: Active L1 table too large
-can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
-can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
+can't open device TEST_DIR/t.qcow2: Active L1 table offset invalid
+can't open device TEST_DIR/t.qcow2: Active L1 table offset invalid
== Invalid L1 table (with internal snapshot in the image) ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v2 2/7] qcow2: Check L1 table offset in qcow2_snapshot_load_tmp()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 1/7] qcow2: Generalize validate_table_offset() into qcow2_validate_table() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters() Alberto Garcia
` (5 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
This function checks that the size of a snapshot's L1 table is not too
large, but it doesn't validate the offset.
We now have a function to take care of this, so let's use it.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-snapshot.c | 8 +++++---
tests/qemu-iotests/080 | 10 +++++++++-
tests/qemu-iotests/080.out | 8 +++++++-
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index cee25f582b..bf94f4f434 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -704,9 +704,11 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
sn = &s->snapshots[snapshot_index];
/* Allocate and read in the snapshot's L1 table */
- if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
- error_setg(errp, "Snapshot L1 table too large");
- return -EFBIG;
+ ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size,
+ sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ "Snapshot L1 table", errp);
+ if (ret < 0) {
+ return ret;
}
new_l1_bytes = sn->l1_size * sizeof(uint64_t);
new_l1_table = qemu_try_blockalign(bs->file->bs,
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 1c2bd85742..6a10e7defa 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -171,7 +171,15 @@ poke_file "$TEST_IMG" "$offset_l2_table_0" "\x80\x00\x00\xff\xff\xff\x00\x00"
{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
echo
-echo "== Invalid snapshot L1 table =="
+echo "== Invalid snapshot L1 table offset =="
+_make_test_img 64M
+{ $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir
+poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00"
+{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
+
+echo
+echo "== Invalid snapshot L1 table size =="
_make_test_img 64M
{ $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 4c7790c9ee..f0d9038d55 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -59,7 +59,13 @@ wrote 512/512 bytes at offset 0
qemu-img: Could not create snapshot 'test': -27 (File too large)
qemu-img: Could not create snapshot 'test': -11 (Resource temporarily unavailable)
-== Invalid snapshot L1 table ==
+== Invalid snapshot L1 table offset ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
+
+== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v2 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 1/7] qcow2: Generalize validate_table_offset() into qcow2_validate_table() Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 2/7] qcow2: Check L1 table offset in qcow2_snapshot_load_tmp() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-07 18:47 ` Eric Blake
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 4/7] qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap() Alberto Garcia
` (4 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
This function iterates over all snapshots of a qcow2 file in order to
expand all zero clusters, but it does not validate the snapshots' L1
tables first.
We now have a function to take care of this, so let's use it.
We can also take the opportunity to replace the sector-based
bdrv_read() with bdrv_pread().
Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Eric Blake <eblake@redhat.com>
---
block/qcow2-cluster.c | 24 +++++++++++++++++-------
tests/qemu-iotests/080 | 2 ++
tests/qemu-iotests/080.out | 4 ++++
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 98908c4264..1aee726c6a 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include <zlib.h>
+#include "qapi/error.h"
#include "qemu-common.h"
#include "block/block_int.h"
#include "block/qcow2.h"
@@ -2092,11 +2093,21 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
}
for (i = 0; i < s->nb_snapshots; i++) {
- int l1_sectors = DIV_ROUND_UP(s->snapshots[i].l1_size *
- sizeof(uint64_t), BDRV_SECTOR_SIZE);
+ int l1_size2;
+ uint64_t *new_l1_table;
+ Error *local_err = NULL;
- uint64_t *new_l1_table =
- g_try_realloc(l1_table, l1_sectors * BDRV_SECTOR_SIZE);
+ ret = qcow2_validate_table(bs, s->snapshots[i].l1_table_offset,
+ s->snapshots[i].l1_size, sizeof(uint64_t),
+ QCOW_MAX_L1_SIZE, "Snapshot L1 table",
+ &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ goto fail;
+ }
+
+ l1_size2 = s->snapshots[i].l1_size * sizeof(uint64_t);
+ new_l1_table = g_try_realloc(l1_table, l1_size2);
if (!new_l1_table) {
ret = -ENOMEM;
@@ -2105,9 +2116,8 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
l1_table = new_l1_table;
- ret = bdrv_read(bs->file,
- s->snapshots[i].l1_table_offset / BDRV_SECTOR_SIZE,
- (void *)l1_table, l1_sectors);
+ ret = bdrv_pread(bs->file, s->snapshots[i].l1_table_offset,
+ l1_table, l1_size2);
if (ret < 0) {
goto fail;
}
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 6a10e7defa..5622604f83 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -177,6 +177,7 @@ _make_test_img 64M
{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir
poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00"
{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
+{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
echo
echo "== Invalid snapshot L1 table size =="
@@ -185,6 +186,7 @@ _make_test_img 64M
{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir
poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
+{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index f0d9038d55..315edbc26f 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -64,10 +64,14 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
+qemu-img: Snapshot L1 table offset invalid
+qemu-img: Error while amending options: Invalid argument
== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
qemu-img: Failed to load snapshot: Snapshot L1 table too large
+qemu-img: Snapshot L1 table too large
+qemu-img: Error while amending options: File too large
*** done
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters() Alberto Garcia
@ 2018-03-07 18:47 ` Eric Blake
0 siblings, 0 replies; 11+ messages in thread
From: Eric Blake @ 2018-03-07 18:47 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz
On 03/06/2018 10:14 AM, Alberto Garcia wrote:
> This function iterates over all snapshots of a qcow2 file in order to
> expand all zero clusters, but it does not validate the snapshots' L1
> tables first.
>
> We now have a function to take care of this, so let's use it.
>
> We can also take the opportunity to replace the sector-based
> bdrv_read() with bdrv_pread().
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> block/qcow2-cluster.c | 24 +++++++++++++++++-------
> tests/qemu-iotests/080 | 2 ++
> tests/qemu-iotests/080.out | 4 ++++
> 3 files changed, 23 insertions(+), 7 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 4/7] qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
` (2 preceding siblings ...)
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto() Alberto Garcia
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
The inactive-l2 overlap check iterates uses the L1 tables from all
snapshots, but it does not validate them first.
We now have a function to take care of this, so let's use it.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 10 +++++++++-
tests/qemu-iotests/080 | 4 ++++
tests/qemu-iotests/080.out | 4 ++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 126cca3276..443ec0a6e5 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2614,9 +2614,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
uint64_t l1_ofs = s->snapshots[i].l1_table_offset;
uint32_t l1_sz = s->snapshots[i].l1_size;
uint64_t l1_sz2 = l1_sz * sizeof(uint64_t);
- uint64_t *l1 = g_try_malloc(l1_sz2);
+ uint64_t *l1;
int ret;
+ ret = qcow2_validate_table(bs, l1_ofs, l1_sz, sizeof(uint64_t),
+ QCOW_MAX_L1_SIZE, "", NULL);
+ if (ret < 0) {
+ return ret;
+ }
+
+ l1 = g_try_malloc(l1_sz2);
+
if (l1_sz2 && l1 == NULL) {
return -ENOMEM;
}
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 5622604f83..018f815697 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -178,6 +178,8 @@ _make_test_img 64M
poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00"
{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
+{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
+ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
echo
echo "== Invalid snapshot L1 table size =="
@@ -187,6 +189,8 @@ _make_test_img 64M
poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir
{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
+{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
+ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 315edbc26f..d622ac06c0 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -66,6 +66,8 @@ wrote 512/512 bytes at offset 0
qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
qemu-img: Snapshot L1 table offset invalid
qemu-img: Error while amending options: Invalid argument
+Failed to flush the refcount block cache: Invalid argument
+write failed: Invalid argument
== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -74,4 +76,6 @@ wrote 512/512 bytes at offset 0
qemu-img: Failed to load snapshot: Snapshot L1 table too large
qemu-img: Snapshot L1 table too large
qemu-img: Error while amending options: File too large
+Failed to flush the refcount block cache: File too large
+write failed: File too large
*** done
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v2 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
` (3 preceding siblings ...)
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 4/7] qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-07 19:10 ` Eric Blake
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 6/7] qcow2: Check snapshot L1 table in qcow2_snapshot_delete() Alberto Garcia
` (2 subsequent siblings)
7 siblings, 1 reply; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
This function copies a snapshot's L1 table into the active one without
validating it first.
We now have a function to take care of this, so let's use it.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Eric Blake <eblake@redhat.com>
---
block/qcow2-snapshot.c | 9 +++++++++
tests/qemu-iotests/080 | 2 ++
tests/qemu-iotests/080.out | 4 ++++
3 files changed, 15 insertions(+)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bf94f4f434..0faf728dc4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -465,6 +465,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
{
BDRVQcow2State *s = bs->opaque;
QCowSnapshot *sn;
+ Error *local_err = NULL;
int i, snapshot_index;
int cur_l1_bytes, sn_l1_bytes;
int ret;
@@ -477,6 +478,14 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
}
sn = &s->snapshots[snapshot_index];
+ ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size,
+ sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ "Snapshot L1 table", &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ goto fail;
+ }
+
if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
error_report("qcow2: Loading snapshots with different disk "
"size is not implemented");
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 018f815697..538857310f 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -180,6 +180,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0
{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
echo
echo "== Invalid snapshot L1 table size =="
@@ -191,6 +192,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir
{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index d622ac06c0..1525e1b087 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -68,6 +68,8 @@ qemu-img: Snapshot L1 table offset invalid
qemu-img: Error while amending options: Invalid argument
Failed to flush the refcount block cache: Invalid argument
write failed: Invalid argument
+qemu-img: Snapshot L1 table offset invalid
+qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument
== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -78,4 +80,6 @@ qemu-img: Snapshot L1 table too large
qemu-img: Error while amending options: File too large
Failed to flush the refcount block cache: File too large
write failed: File too large
+qemu-img: Snapshot L1 table too large
+qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large
*** done
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto()
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto() Alberto Garcia
@ 2018-03-07 19:10 ` Eric Blake
0 siblings, 0 replies; 11+ messages in thread
From: Eric Blake @ 2018-03-07 19:10 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz
On 03/06/2018 10:14 AM, Alberto Garcia wrote:
> This function copies a snapshot's L1 table into the active one without
> validating it first.
>
> We now have a function to take care of this, so let's use it.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> block/qcow2-snapshot.c | 9 +++++++++
> tests/qemu-iotests/080 | 2 ++
> tests/qemu-iotests/080.out | 4 ++++
> 3 files changed, 15 insertions(+)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 6/7] qcow2: Check snapshot L1 table in qcow2_snapshot_delete()
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
` (4 preceding siblings ...)
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 7/7] qcow2: Make qemu-img check detect corrupted L1 tables in snapshots Alberto Garcia
2018-03-06 17:04 ` [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Kevin Wolf
7 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
This function deletes a snapshot from disk, removing its entry from
the snapshot table, freeing its L1 table and decreasing the refcounts
of all clusters.
The L1 table offset and size are however not validated. If we use
invalid values in this function we'll probably corrupt the image even
more, so we should return an error instead.
We now have a function to take care of this, so let's use it.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-snapshot.c | 7 +++++++
tests/qemu-iotests/080 | 2 ++
tests/qemu-iotests/080.out | 2 ++
3 files changed, 11 insertions(+)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0faf728dc4..74293be470 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -611,6 +611,13 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
}
sn = s->snapshots[snapshot_index];
+ ret = qcow2_validate_table(bs, sn.l1_table_offset, sn.l1_size,
+ sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ "Snapshot L1 table", errp);
+ if (ret < 0) {
+ return ret;
+ }
+
/* Remove it from the snapshot list */
memmove(s->snapshots + snapshot_index,
s->snapshots + snapshot_index + 1,
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 538857310f..f8e7d6f4df 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -181,6 +181,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0
{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
+{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
echo
echo "== Invalid snapshot L1 table size =="
@@ -193,6 +194,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
+{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 1525e1b087..89bcd27172 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -70,6 +70,7 @@ Failed to flush the refcount block cache: Invalid argument
write failed: Invalid argument
qemu-img: Snapshot L1 table offset invalid
qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument
+qemu-img: Could not delete snapshot 'test': Snapshot L1 table offset invalid
== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -82,4 +83,5 @@ Failed to flush the refcount block cache: File too large
write failed: File too large
qemu-img: Snapshot L1 table too large
qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large
+qemu-img: Could not delete snapshot 'test': Snapshot L1 table too large
*** done
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH v2 7/7] qcow2: Make qemu-img check detect corrupted L1 tables in snapshots
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
` (5 preceding siblings ...)
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 6/7] qcow2: Check snapshot L1 table in qcow2_snapshot_delete() Alberto Garcia
@ 2018-03-06 16:14 ` Alberto Garcia
2018-03-06 17:04 ` [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Kevin Wolf
7 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2018-03-06 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Kevin Wolf, Max Reitz, Eric Blake
'qemu-img check' cannot detect if a snapshot's L1 table is corrupted.
This patch checks the table's offset and size and reports corruption
if the values are not valid.
This patch doesn't add code to fix that corruption yet, only to detect
and report it.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 14 ++++++++++++++
tests/qemu-iotests/080 | 2 ++
tests/qemu-iotests/080.out | 20 ++++++++++++++++++++
3 files changed, 36 insertions(+)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 443ec0a6e5..9d4b4c29d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2019,6 +2019,20 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
/* snapshots */
for (i = 0; i < s->nb_snapshots; i++) {
sn = s->snapshots + i;
+ if (offset_into_cluster(s, sn->l1_table_offset)) {
+ fprintf(stderr, "ERROR snapshot %s (%s) l1_offset=%#" PRIx64 ": "
+ "L1 table is not cluster aligned; snapshot table entry "
+ "corrupted\n", sn->id_str, sn->name, sn->l1_table_offset);
+ res->corruptions++;
+ continue;
+ }
+ if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
+ fprintf(stderr, "ERROR snapshot %s (%s) l1_size=%#" PRIx32 ": "
+ "L1 table is too large; snapshot table entry corrupted\n",
+ sn->id_str, sn->name, sn->l1_size);
+ res->corruptions++;
+ continue;
+ }
ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
sn->l1_table_offset, sn->l1_size, 0, fix);
if (ret < 0) {
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index f8e7d6f4df..4dbe68e950 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -182,6 +182,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
+_check_test_img
echo
echo "== Invalid snapshot L1 table size =="
@@ -195,6 +196,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
-c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir
{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir
{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
+_check_test_img
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 89bcd27172..4e0f7f7b92 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -71,6 +71,16 @@ write failed: Invalid argument
qemu-img: Snapshot L1 table offset invalid
qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument
qemu-img: Could not delete snapshot 'test': Snapshot L1 table offset invalid
+ERROR snapshot 1 (test) l1_offset=0x400200: L1 table is not cluster aligned; snapshot table entry corrupted
+Leaked cluster 4 refcount=2 reference=1
+Leaked cluster 5 refcount=2 reference=1
+Leaked cluster 6 refcount=1 reference=0
+
+1 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+
+3 leaked clusters were found on the image.
+This means waste of disk space, but no harm to data.
== Invalid snapshot L1 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
@@ -84,4 +94,14 @@ write failed: File too large
qemu-img: Snapshot L1 table too large
qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large
qemu-img: Could not delete snapshot 'test': Snapshot L1 table too large
+ERROR snapshot 1 (test) l1_size=0x10000000: L1 table is too large; snapshot table entry corrupted
+Leaked cluster 4 refcount=2 reference=1
+Leaked cluster 5 refcount=2 reference=1
+Leaked cluster 6 refcount=1 reference=0
+
+1 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+
+3 leaked clusters were found on the image.
+This means waste of disk space, but no harm to data.
*** done
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table
2018-03-06 16:14 [Qemu-devel] [PATCH v2 0/7] Add checks for corruption in the snapshot table Alberto Garcia
` (6 preceding siblings ...)
2018-03-06 16:14 ` [Qemu-devel] [PATCH v2 7/7] qcow2: Make qemu-img check detect corrupted L1 tables in snapshots Alberto Garcia
@ 2018-03-06 17:04 ` Kevin Wolf
7 siblings, 0 replies; 11+ messages in thread
From: Kevin Wolf @ 2018-03-06 17:04 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz, Eric Blake
Am 06.03.2018 um 17:14 hat Alberto Garcia geschrieben:
> Hey,
>
> here's the new version of this series. It fixes a leak reported by
> Kevin and adds a couple of error_report_err() to make use of the
> message returned by qcow2_validate_table().
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread