Add checks for corruption in the snapshot table
PATCH 4/7 qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap()
In reply to Alberto GarciaThe 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.
--- block/qcow2-refcount.c | 10 +++++++++- tests/qemu-iotests/080 | 4 ++++ tests/qemu-iotests/080.out | 4 ++++ 3 files changed, 17 insertions(+), 1 deletion(-)
block/qcow2-refcount.c +9 -1
@@ -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 ; }
tests/qemu-iotests/080 +4 -0
@@ -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"
tests/qemu-iotests/080.out +4 -0
@@ -65,6 +65,8 @@ 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: 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
@@ -72,4 +74,6 @@ 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: Error while amending options: File too large +Failed to flush the refcount block cache: File too large +write failed: File too large *** done
Signature · 1 line
2.11.0
PATCH 5/7 qcow2: Check snapshot L1 table in qcow2_snapshot_goto()
In reply to Alberto GarciaThis 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.
--- block/qcow2-snapshot.c | 6 ++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 10 insertions(+)
block/qcow2-snapshot.c +6 -0
@@ -477,6 +477,12 @@ 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 ,"" ,NULL ); +if (ret <0 ) { + goto fail; + } +if (sn->disk_size != bs->total_sectors *BDRV_SECTOR_SIZE ) {error_report ("qcow2: Loading snapshots with different disk " "size is not implemented" );
tests/qemu-iotests/080 +2 -0
@@ -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"
tests/qemu-iotests/080.out +2 -0
@@ -67,6 +67,7 @@ qemu-img: Failed to load snapshot: 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: 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
@@ -76,4 +77,5 @@ qemu-img: Failed to load snapshot: 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: Could not apply snapshot 'test': Failed to load snapshot: File too large *** done
Signature · 1 line
2.11.0
PATCH 6/7 qcow2: Check snapshot L1 table in qcow2_snapshot_delete()
In reply to Alberto GarciaThis 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.
--- block/qcow2-snapshot.c | 7 +++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 11 insertions(+)
block/qcow2-snapshot.c +7 -0
@@ -608,6 +608,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,
tests/qemu-iotests/080 +2 -0
@@ -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"
tests/qemu-iotests/080.out +2 -0
@@ -68,6 +68,7 @@ qemu-img: Error while amending options: Invalid argument Failed to flush the refcount block cache: Invalid argument write failed: Invalid argument 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
@@ -78,4 +79,5 @@ 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: 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
Signature · 1 line
2.11.0
cover Add checks for corruption in the snapshot table
Hey ho!
As we have already discussed in the mailing list, the offset and size values of snapshots' L1 tables are almost never validated in the QEMU code.
One way to deal with this is to validate them when the image is being opened (in qcow2_read_snapshots()) and return an error if there's anything wrong. However this would render the image unusable because we wouldn't be able to try to recover data using the active table as we can do now.
Another possibility is to refuse opening the image but add a way to fix it using 'qemu-img check'. But this would be a destructive operation, and knowing that the image is already corrupted we can't guarantee that we wouldn't be corrupting it even more.
So although having a way to repair this kind of corruption would be nice, we should still allow the user to try to open the image and read data from it without requiring a potentially destructive operation first.
So this series simply validates the snapshots' L1 tables in the places where they are actually used. We already have a function to do this kind of checks so we only need to call it.
Regards,
Berto
Alberto Garcia (7):
qcow2: Generalize validate_table_offset() into qcow2_validate_table()
qcow2: Check L1 table offset in qcow2_snapshot_load_tmp()
qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap()
qcow2: Check snapshot L1 table in qcow2_snapshot_goto()
qcow2: Check snapshot L1 table in qcow2_snapshot_delete()
qcow2: Make qemu-img check detect corrupted L1 tables in snapshots
block/qcow2-cluster.c | 20 +++++++----- block/qcow2-refcount.c | 24 ++++++++++++++- block/qcow2-snapshot.c | 21 +++++++++++-- block/qcow2.c | 77 ++++++++++++++++++---------------------------- block/qcow2.h | 10 +++--- tests/qemu-iotests/080 | 22 ++++++++++++- tests/qemu-iotests/080.out | 54 ++++++++++++++++++++++++++------ 7 files changed, 155 insertions(+), 73 deletions(-)
Signature · 1 line
2.11.0
PATCH 1/7 qcow2: Generalize validate_table_offset() into qcow2_validate_table()
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.
--- block/qcow2.c | 77 ++++++++++++++++++---------------------------- block/qcow2.h | 10 +++--- tests/qemu-iotests/080.out | 16 +++++----- 3 files changed, 43 insertions(+), 60 deletions(-)
block/qcow2.c +30 -47
@@ -561,26 +561,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 ; }
@@ -1310,47 +1307,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 ) {
@@ -1368,15 +1360,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,align_offset (s->l1_size *sizeof (uint64_t ),512 ));
block/qcow2.h +5 -5
@@ -491,11 +491,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 ) {
@@ -553,6 +548,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);
tests/qemu-iotests/080.out +8 -8
@@ -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
Signature · 1 line
2.11.0
PATCH 2/7 qcow2: Check L1 table offset in qcow2_snapshot_load_tmp()
In reply to Alberto GarciaThis 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.
--- block/qcow2-snapshot.c | 8 +++++--- tests/qemu-iotests/080 | 10 +++++++++- tests/qemu-iotests/080.out | 8 +++++++- 3 files changed, 21 insertions(+), 5 deletions(-)
block/qcow2-snapshot.c +5 -3
@@ -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,
tests/qemu-iotests/080 +9 -1
@@ -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
tests/qemu-iotests/080.out +7 -1
@@ -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)
Signature · 1 line
2.11.0
PATCH 7/7 qcow2: Make qemu-img check detect corrupted L1 tables in snapshots
In reply to Alberto Garcia'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.
--- block/qcow2-refcount.c | 14 ++++++++++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+)
block/qcow2-refcount.c +14 -0
@@ -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 ) {
tests/qemu-iotests/080 +2 -0
@@ -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"
tests/qemu-iotests/080.out +20 -0
@@ -69,6 +69,16 @@ Failed to flush the refcount block cache: Invalid argument write failed: Invalid argument 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
@@ -80,4 +90,14 @@ Failed to flush the refcount block cache: File too large write failed: File 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
Signature · 1 line
2.11.0
PATCH 3/7 qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
In reply to Alberto GarciaThis 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().
--- block/qcow2-cluster.c | 20 +++++++++++++------- tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 17 insertions(+), 7 deletions(-)
block/qcow2-cluster.c +13 -7
@@ -2092,11 +2092,18 @@ 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; -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 ,"" ,NULL ); +if (ret <0 ) { +return ret; + } + + 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 +2112,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; }
tests/qemu-iotests/080 +2 -0
@@ -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"
tests/qemu-iotests/080.out +2 -0
@@ -64,10 +64,12 @@ 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: 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: Error while amending options: File too large *** done
Signature · 1 line
2.11.0
Re: [Qemu-devel] [PATCH 1/7] qcow2: Generalize validate_table_offset() into qcow2_validate_table()
In reply to Alberto GarciaQuoted 20 lines · Alberto Garcia
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 <[email protected]>---
block/qcow2.c +2
-0+int qcow2_validate_table(BlockDriverState *bs, uint64_t offset, + uint64_t entries, size_t entry_len,
block/qcow2.c static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result, · line 574
Quoted 10 lines
block/qcow2.c +7
-1+ 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 ; + }
EFBIG "File too large". Would EOVERFLOW "Value too large for defined data type" make any more sense? But that's bikeshedding; I'm okay with your choice.
block/qcow2.c static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, · line 1341
Quoted 9 lines
/* read the level 1 table */block/qcow2.c +4
-3-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 ) {
At any rate, it looks like we were already using EFBIG.
I also like that you consolidated more checking into the common function.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/7] qcow2: Check L1 table offset in qcow2_snapshot_load_tmp()
In reply to Alberto GarciaQuoted 12 lines · Alberto Garcia
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 <[email protected]>--- block/qcow2-snapshot.c | 8 +++++--- tests/qemu-iotests/080 | 10 +++++++++- tests/qemu-iotests/080.out | 8 +++++++- 3 files changed, 21 insertions(+), 5 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
In reply to Alberto GarciaQuoted 8 lines · Alberto Garcia
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()withbdrv_pread().
Doesn't my pending patch do that as well?
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06799.html
I guess it remains to be seen in what order these patches are merged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- block/qcow2-cluster.c | 20 +++++++++++++------- tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 17 insertions(+), 7 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 4/7] qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap()
In reply to Alberto GarciaQuoted 11 lines · Alberto Garcia
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 <[email protected]>--- block/qcow2-refcount.c | 10 +++++++++- tests/qemu-iotests/080 | 4 ++++ tests/qemu-iotests/080.out | 4 ++++ 3 files changed, 17 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 5/7] qcow2: Check snapshot L1 table in qcow2_snapshot_goto()
In reply to Alberto GarciaQuoted 12 lines · Alberto Garcia
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 <[email protected]>--- block/qcow2-snapshot.c | 6 ++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 10 insertions(+)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 6/7] qcow2: Check snapshot L1 table in qcow2_snapshot_delete()
In reply to Alberto GarciaQuoted 8 lines · Alberto Garcia
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.
Quoted 9 lines
We now have a function to take care of this, so let's use it.
Signed-off-by Alberto Garcia <[email protected]>--- block/qcow2-snapshot.c | 7 +++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 11 insertions(+)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 7/7] qcow2: Make qemu-img check detect corrupted L1 tables in snapshots
In reply to Alberto GarciaQuoted 7 lines · Alberto Garcia
'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.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- block/qcow2-refcount.c | 14 ++++++++++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 01.03.2018 um 17:27 hat Alberto Garcia geschrieben:
Quoted 17 lines
Hey ho!
As we have already discussed in the mailing list, the offset and size values of snapshots' L1 tables are almost never validated in the QEMU code.
One way to deal with this is to validate them when the image is being opened (in
qcow2_read_snapshots()) and return an error if there's anything wrong. However this would render the image unusable because we wouldn't be able to try to recover data using the active table as we can do now.Another possibility is to refuse opening the image but add a way to fix it using 'qemu-img check'. But this would be a destructive operation, and knowing that the image is already corrupted we can't guarantee that we wouldn't be corrupting it even more.
Quoted 4 lines
So although having a way to repair this kind of corruption would be nice, we should still allow the user to try to open the image and read data from it without requiring a potentially destructive operation first.
Generally speaking, there is also always the option to refuse opening a corrupted image read-write, but to allow opening it read-only. This would probably still require some checks on use, though (or maybe just hiding all snapshots).
Kevin
Quoted 29 lines
So this series simply validates the snapshots' L1 tables in the places where they are actually used. We already have a function to do this kind of checks so we only need to call it.
Regards,
Berto
Alberto Garcia (7):
qcow2: Generalizevalidate_table_offset()intoqcow2_validate_table()
qcow2: Check L1 table offset inqcow2_snapshot_load_tmp()
qcow2: Check L1 table parameters inqcow2_expand_zero_clusters()
qcow2: Check snapshot L1 tables inqcow2_check_metadata_overlap()
qcow2: Check snapshot L1 table inqcow2_snapshot_goto()
qcow2: Check snapshot L1 table inqcow2_snapshot_delete()
qcow2: Make qemu-img check detect corrupted L1 tables in snapshotsblock/qcow2-cluster.c | 20 +++++++----- block/qcow2-refcount.c | 24 ++++++++++++++- block/qcow2-snapshot.c | 21 +++++++++++-- block/qcow2.c | 77 ++++++++++++++++++---------------------------- block/qcow2.h | 10 +++--- tests/qemu-iotests/080 | 22 ++++++++++++- tests/qemu-iotests/080.out | 54 ++++++++++++++++++++++++++------ 7 files changed, 155 insertions(+), 73 deletions(-)Signature · 1 line
2.11.0
Quoted 9 lines · Kevin Wolf
Quoted 4 lines
So although having a way to repair this kind of corruption would be nice, we should still allow the user to try to open the image and read data from it without requiring a potentially destructive operation first.
Generally speaking, there is also always the option to refuse opening a corrupted image read-write, but to allow opening it read-only. This would probably still require some checks on use, though (or maybe just hiding all snapshots).
You're right. The checks are in this series anyway.
Berto
Re: [Qemu-devel] [PATCH 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
In reply to Alberto GarciaAm 01.03.2018 um 17:27 hat Alberto Garcia geschrieben:
Quoted 26 lines
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()withbdrv_pread().Signed-off-by Alberto Garcia <[email protected]>--- block/qcow2-cluster.c | 20 +++++++++++++------- tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 17 insertions(+), 7 deletions(-)block/qcow2-cluster.c +0
-2@@ -2092,11 +2092,18 @@ 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 );
block/qcow2-cluster.c int qcow2_expand_zero_clusters(BlockDriverState *bs, · line 2102
Quoted 10 lines
block/qcow2-cluster.c +7
-2+int l1_size2; +uint64_t *new_l1_table; -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 ,"" ,NULL ); +if (ret <0 ) { +return ret;
Shouldn't this be goto fail?
Kevin
Re: [Qemu-devel] [PATCH 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
Quoted 9 lines · Kevin Wolf
Quoted 9 lines
block/qcow2-cluster.c +2
-2@@ -2092,11 +2092,18 @@ 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;
Quoted 9 lines
block/qcow2-cluster.c int qcow2_expand_zero_clusters(BlockDriverState *bs, · line 2102
Quoted 7 lines
block/qcow2-cluster.c +5
-2-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 ,"" ,NULL ); +if (ret <0 ) { +return ret;Shouldn't this be goto fail?
You're right, this is a loop, and l1_table could have been initialized in previous iterations.
I'll send a corrected version with this change, but first I'll wait a bit in case you see anything else in the series.
Berto
Re: [Qemu-devel] [PATCH 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
Am 06.03.2018 um 16:01 hat Alberto Garcia geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 9 lines
Quoted 9 lines
block/qcow2-cluster.c +2
-2@@ -2092,11 +2092,18 @@ 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;Quoted 9 lines
block/qcow2-cluster.c int qcow2_expand_zero_clusters(BlockDriverState *bs, · line 2102
Quoted 7 lines
block/qcow2-cluster.c +5
-2-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 ,"" ,NULL ); +if (ret <0 ) { +return ret;Shouldn't this be goto fail?
Quoted 5 lines
You're right, this is a loop, and
l1_tablecould have been initialized in previous iterations.I'll send a corrected version with this change, but first I'll wait a bit in case you see anything else in the series.
I've finished the review now, the rest looks correct.
The only other thing I wondered is about the cases where you pass a NULL errp because the callers don't get an Error parameter, so they can't pass it on. Some of these callers already use error_report(), so it would be okay to use error_report_err() for an error returned by qcow2_validate_table(), too. I think that would improve the messages.
Kevin
Re: [Qemu-devel] [PATCH 3/7] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters()
Quoted 7 lines · Kevin Wolf
I've finished the review now, the rest looks correct.
The only other thing I wondered is about the cases where you pass a NULL errp because the callers don't get an Error parameter, so they can't pass it on. Some of these callers already use
error_report(), so it would be okay to useerror_report_err()for an error returned byqcow2_validate_table(), too. I think that would improve the messages.
Good idea, I'll change that and resend the series.
Berto