parallels_open()'s BAT scan and parallels_check_outside_image() only checked entries against the file's upper end, matching just half of what docs/interop/parallels.rst requires: an entry's offset must be both >= data_start and < the file size. An entry below data_start resolves into the header/BAT region itself, corrupting metadata on write or losing the write silently on a partial overlap, and neither qemu-img check nor the open-time scan ever caught it.
Check both bounds everywhere a BAT entry is resolved to a host offset: seek_to_sector(), the open-time scan (without letting a bad entry inflate data_end), and parallels_check_outside_image(). Signed-off-by: Denis V. Lunev <[email protected]> CC: Thomas Huth <[email protected]> CC: Stefan Hajnoczi <[email protected]> --- block/parallels.c | 27 +++++++-- tests/qemu-iotests/tests/parallels-checks | 58 +++++++++++++++++++ tests/qemu-iotests/tests/parallels-checks.out | 35 +++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index e3d26a6650..8a7e8b4aba 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -119,6 +119,7 @@ static uint32_t bat_entry_off(uint32_t idx) static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num) { uint32_t index, offset; + int64_t cluster_off; index = sector_num / s->tracks; offset = sector_num % s->tracks; @@ -127,7 +128,14 @@ static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num) if ((index >= s->bat_size) || (s->bat_bitmap[index] == 0)) { return -1; } - return bat2sect(s, index) + offset; + + cluster_off = bat2sect(s, index); + if (cluster_off < s->data_start || cluster_off + s->tracks > s->data_end) { + /* Cluster is outside of the image file or overlaps the header. */ + return -1; + } + + return cluster_off + offset; } static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num, @@ -703,18 +711,22 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res, { BDRVParallelsState *s = bs->opaque; uint32_t i; - int64_t off, high_off, size; + int64_t off, high_off, size, data_start_off; size = bdrv_co_getlength(bs->file->bs); if (size < 0) { res->check_errors++; return size; } + data_start_off = s->data_start << BDRV_SECTOR_BITS; high_off = 0; for (i = 0; i < s->bat_size; i++) { off = bat2sect(s, i) << BDRV_SECTOR_BITS; - if (off + s->cluster_size > size) { + if (off == 0) { + continue; + } + if (off < data_start_off || off + s->cluster_size > size) { fprintf(stderr, "%s cluster %u is outside image\n", fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); res->corruptions++; @@ -1398,11 +1410,18 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, for (i = 0; i < s->bat_size; i++) { sector = bat2sect(s, i); + if (sector == 0) { + continue; /* not allocated */ + } + if (sector < data_start || sector + s->tracks > file_nb_sectors) { + /* Cluster is outside of the image file or overlaps the header. */ + need_check = true; + continue; + } if (sector + s->tracks > s->data_end) { s->data_end = sector + s->tracks; } } - need_check = need_check || s->data_end > file_nb_sectors; if (!need_check) { ret = parallels_fill_used_bitmap(bs); diff --git a/tests/qemu-iotests/tests/parallels-checks b/tests/qemu-iotests/tests/parallels-checks index 9535024885..abd119bc7b 100755 --- a/tests/qemu-iotests/tests/parallels-checks +++ b/tests/qemu-iotests/tests/parallels-checks @@ -222,6 +222,64 @@ _img_info echo "== an unallocated cluster still reads as zeroes ==" { $QEMU_IO -r -c "read -P 0x00 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir +# Clear image +_make_test_img $SIZE + +echo "== TEST BAT ENTRY POINTING OUTSIDE IMAGE ==" + +echo "== corrupt image: point first cluster far outside the file ==" +poke_file_le "$TEST_IMG" $BAT_OFFSET 4 1000000 + +echo "== read-only read must return zeroes, not an I/O error ==" +{ $QEMU_IO -r -c "read -P 0x00 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir + +echo "== write must allocate a fresh cluster instead of trusting the entry ==" +{ $QEMU_IO -c "write -P 0x77 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir + +echo "== file did not grow anywhere near the bogus offset ==" +file_size=`stat --printf="%s" "$TEST_IMG"` +if [ "$file_size" -lt $((16 * 1024 * 1024)) ]; then + echo "file size sane: yes" +else + echo "file size sane: no ($file_size bytes)" +fi + +echo "== data reads back correctly ==" +{ $QEMU_IO -r -c "read -P 0x77 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir + +# Clear image, with a small cluster size so the BAT table itself spans +# more than one cluster and there is room to point before data_off. +_make_test_img -o cluster_size=512 65536 + +SMALL_CLUSTER_SIZE=$(peek_file_le $TEST_IMG $CLUSTER_SIZE_OFFSET 4) +SMALL_CLUSTER_SIZE=$((SMALL_CLUSTER_SIZE * 512)) +DATA_OFF=$(peek_file_le $TEST_IMG $DATA_OFF_OFFSET 4) +echo "cluster size: $SMALL_CLUSTER_SIZE, data offset (sectors): $DATA_OFF" + +# Cluster index 1 starts at this byte offset, which must be < data_off +# in sectors * 512 for this test to actually exercise the bug. +VICTIM_OFFSET=$SMALL_CLUSTER_SIZE + +echo "== TEST BAT ENTRY POINTING BEFORE DATA AREA ==" + +echo "== corrupt image: point first cluster into the BAT table itself ==" +poke_file_le "$TEST_IMG" $BAT_OFFSET 4 1 + +echo "== qemu-img check detects it without repairing ==" +_check_test_img + +echo "== bytes at the victim offset before write ==" +echo "$(peek_file_le "$TEST_IMG" $VICTIM_OFFSET 4)" + +echo "== write must allocate a fresh cluster instead of clobbering the BAT ==" +{ $QEMU_IO -c "write -P 0x88 0 $SMALL_CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir + +echo "== bytes at the victim offset are unchanged ==" +echo "$(peek_file_le "$TEST_IMG" $VICTIM_OFFSET 4)" + +echo "== data reads back correctly ==" +{ $QEMU_IO -r -c "read -P 0x88 0 $SMALL_CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/tests/parallels-checks.out b/tests/qemu-iotests/tests/parallels-checks.out index 6fb2014e8e..914616d023 100644 --- a/tests/qemu-iotests/tests/parallels-checks.out +++ b/tests/qemu-iotests/tests/parallels-checks.out @@ -140,4 +140,39 @@ virtual size: 4 MiB (4194304 bytes) == an unallocated cluster still reads as zeroes == read 1048576/1048576 bytes at offset 0 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304 +== TEST BAT ENTRY POINTING OUTSIDE IMAGE == +== corrupt image: point first cluster far outside the file == +== read-only read must return zeroes, not an I/O error == +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== write must allocate a fresh cluster instead of trusting the entry == +Repairing cluster 0 is outside image +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== file did not grow anywhere near the bogus offset == +file size sane: yes +== data reads back correctly == +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 +cluster size: 512, data offset (sectors): 2 +== TEST BAT ENTRY POINTING BEFORE DATA AREA == +== corrupt image: point first cluster into the BAT table itself == +== qemu-img check detects it without repairing == +ERROR cluster 0 is outside image + +1 errors were found on the image. +Data may be corrupted, or further writes to the image may corrupt it. +== bytes at the victim offset before write == +0 +== write must allocate a fresh cluster instead of clobbering the BAT == +Repairing cluster 0 is outside image +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== bytes at the victim offset are unchanged == +0 +== data reads back correctly == +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done -- 2.53.0
