raw: Prohibit dangerous writes for probed images

Reviewed-by Eric Blake <[email protected]>

---
 qemu-io.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

qemu-io.c +20 -8

index 60f84dd..91a445a 100644
@@ -51,7 +51,8 @@ static const cmdinfo_t close_cmd = {
     .oneline    = "close the current open file",
 };
 
-static int openfile(char *name, int flags, int growable, QDict *opts)
+static int openfile(char *name, BlockDriver *drv, int flags, int growable,
+                    QDict *opts)
 {
     Error *local_err = NULL;

@@ -68,7 +69,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
         flags |= BDRV_O_PROTOCOL;
     }
 
-    if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err) < 0) {
+    if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, drv, &local_err) < 0) {
         fprintf(stderr, "%s: can't open%s%s: %s\n", progname,
                 name ? " device " : "", name ?: "",
                 error_get_pretty(local_err));

@@ -169,9 +170,9 @@ static int open_f(BlockDriverState *bs, int argc, char **argv)
     qemu_opts_reset(&empty_opts);
 
     if (optind == argc - 1) {
-        return openfile(argv[optind], flags, growable, opts);
+        return openfile(argv[optind], NULL, flags, growable, opts);
     } else if (optind == argc) {
-        return openfile(NULL, flags, growable, opts);
+        return openfile(NULL, NULL, flags, growable, opts);
     } else {
         QDECREF(opts);
         return qemuio_command_usage(&open_cmd);

@@ -196,11 +197,12 @@ static const cmdinfo_t quit_cmd = {
 static void usage(const char *name)
 {
     printf(
-"Usage: %s [-h] [-V] [-rsnm] [-c STRING] ... [file]\n"
+"Usage: %s [-h] [-V] [-rsnm] [-f FMT] [-c STRING] ... [file]\n"
 "QEMU Disk exerciser\n"
 "\n"
 "  -c, --cmd STRING     execute command with its arguments\n"
 "                       from the given string\n"
+"  -f, --format FMT     specifies the block driver to use\n"
 "  -r, --read-only      export read-only\n"
 "  -s, --snapshot       use snapshot file\n"
 "  -n, --nocache        disable host cache\n"

@@ -364,12 +366,13 @@ int main(int argc, char **argv)
 {
     int readonly = 0;
     int growable = 0;
-    const char *sopt = "hVc:d:rsnmgkt:T:";
+    const char *sopt = "hVc:d:f:rsnmgkt:T:";
     const struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
         { "offset", 1, NULL, 'o' },
         { "cmd", 1, NULL, 'c' },
+        { "format", 1, NULL, 'f' },
         { "read-only", 0, NULL, 'r' },
         { "snapshot", 0, NULL, 's' },
         { "nocache", 0, NULL, 'n' },

@@ -384,6 +387,7 @@ int main(int argc, char **argv)
     int c;
     int opt_index = 0;
     int flags = BDRV_O_UNMAP;
+    BlockDriver *drv = NULL;
     Error *local_error = NULL;
 
 #ifdef CONFIG_POSIX

@@ -393,6 +397,8 @@ int main(int argc, char **argv)
     progname = basename(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
+    bdrv_init();
+
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
         switch (c) {
         case 's':

@@ -407,6 +413,13 @@ int main(int argc, char **argv)
                 exit(1);
             }
             break;
+        case 'f':
+            drv = bdrv_find_format(optarg);
+            if (!drv) {
+                error_report("Invalid format '%s'", optarg);
+                exit(EXIT_FAILURE);
+            }
+            break;
         case 'c':
             add_user_command(optarg);
             break;

@@ -455,7 +468,6 @@ int main(int argc, char **argv)
         error_free(local_error);
         exit(1);
     }
-    bdrv_init();
 
     /* initialize commands */
     qemuio_add_command(&quit_cmd);

@@ -477,7 +489,7 @@ int main(int argc, char **argv)
     }
 
     if ((argc - optind) == 1) {
-        openfile(argv[optind], flags, growable, NULL);
+        openfile(argv[optind], drv, flags, growable, NULL);
     }
     command_loop();

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #2 lore raw

PATCH v3 2/9 qemu-iotests: Use qemu-io -f $IMGFMT

This patch changes $QEMU_IO so that all tests by default pass a format argument to qemu-io.

There are a few cases where -f $IMGFMT is not wanted because it selects the wrong driver or json: filenames including a driver are used. They are changed to use $QEMU_IO_PROG, which doesn't include any options.

Tests 071 and 081 have output changes because now the actual request fails instead of reading the 2k probing buffer.

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

---
 tests/qemu-iotests/016     | 11 +++++++----
 tests/qemu-iotests/048     |  2 +-
 tests/qemu-iotests/058     | 11 +++++++----
 tests/qemu-iotests/071     | 10 +++++-----
 tests/qemu-iotests/071.out |  6 +++---
 tests/qemu-iotests/077     |  2 +-
 tests/qemu-iotests/081     |  8 ++++++--
 tests/qemu-iotests/081.out |  2 +-
 tests/qemu-iotests/089     |  6 ++++--
 tests/qemu-iotests/common  |  2 +-
 10 files changed, 36 insertions(+), 24 deletions(-)

tests/qemu-iotests/016 +7 -4

index 7ea9e94..52397aa 100755
@@ -43,25 +43,28 @@ _supported_proto file sheepdog nfs
 _supported_os Linux
 
 
+# No -f, use probing for the protocol driver
+QEMU_IO_PROTO="$QEMU_IO_PROG -g --cache $CACHEMODE"
+
 size=128M
 _make_test_img $size
 
 echo
 echo "== reading at EOF =="
-$QEMU_IO -g -c "read -P 0 $size 512" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO_PROTO -c "read -P 0 $size 512" "$TEST_IMG" | _filter_qemu_io
 
 echo
 echo "== reading far past EOF =="
-$QEMU_IO -g -c "read -P 0 256M 512" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO_PROTO -c "read -P 0 256M 512" "$TEST_IMG" | _filter_qemu_io
 
 echo
 echo "== writing at EOF =="
-$QEMU_IO -g -c "write -P 66 $size 512" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO_PROTO -c "write -P 66 $size 512" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -P 66 $size 512" "$TEST_IMG" | _filter_qemu_io
 
 echo
 echo "== writing far past EOF =="
-$QEMU_IO -g -c "write -P 66 256M 512" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO_PROTO -c "write -P 66 256M 512" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -P 66 256M 512" "$TEST_IMG" | _filter_qemu_io
 
 # success, all done

tests/qemu-iotests/048 +1 -1

index 65da46d..e1eeac2 100755
@@ -64,7 +64,7 @@ _compare
 _compare -q
 
 # Compare images with different size
-$QEMU_IMG resize "$TEST_IMG" +512M
+$QEMU_IMG resize -f $IMGFMT "$TEST_IMG" +512M
 _compare
 _compare -s

tests/qemu-iotests/058 +7 -4

index 14584cd..2d5ca85 100755
@@ -89,6 +89,9 @@ _supported_fmt qcow2
 _supported_proto file
 _require_command QEMU_NBD
 
+# Use -f raw instead of -f $IMGFMT for the NBD connection
+QEMU_IO_NBD="$QEMU_IO -f raw --cache=$CACHEMODE"
+
 echo
 echo "== preparing image =="
 _make_test_img 64M

@@ -108,15 +111,15 @@ _export_nbd_snapshot sn1
 
 echo
 echo "== verifying the exported snapshot with patterns, method 1 =="
-$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
-$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
+$QEMU_IO_NBD -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
+$QEMU_IO_NBD -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
 
 _export_nbd_snapshot1 sn1
 
 echo
 echo "== verifying the exported snapshot with patterns, method 2 =="
-$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
-$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
+$QEMU_IO_NBD -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
+$QEMU_IO_NBD -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" | _filter_qemu_io
 
 $QEMU_IMG convert "$TEST_IMG" -l sn1 -O qcow2 "$converted_image"

tests/qemu-iotests/071 +5 -5

index 3924e51..5d61ef6 100755
@@ -63,12 +63,12 @@ echo
 TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
     _filter_imgfmt
 _make_test_img $IMG_SIZE
-$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+$QEMU_IO -c "open -o driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
          -c 'read 0 512' -c 'write -P 42 0x38000 512' -c 'read -P 42 0x38000 512' | _filter_qemu_io
 
 $QEMU_IO -c 'write -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
 
-$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+$QEMU_IO -c "open -o driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
          -c 'read -P 42 0 512' | _filter_qemu_io
 
 echo

@@ -78,12 +78,12 @@ echo
 TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
     _filter_imgfmt
 _make_test_img $IMG_SIZE
-$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base,file.test.driver=$IMGFMT,file.test.file.filename=$TEST_IMG" \
+$QEMU_IO -c "open -o driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base,file.test.driver=$IMGFMT,file.test.file.filename=$TEST_IMG" \
          -c 'read 0 512' -c 'write -P 42 0x38000 512' -c 'read -P 42 0x38000 512' | _filter_qemu_io
 
 $QEMU_IO -c 'write -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
 
-$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+$QEMU_IO -c "open -o driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
          -c 'read -P 42 0 512' | _filter_qemu_io
 
 echo

@@ -163,7 +163,7 @@ echo
 echo "=== Testing blkverify on existing raw block device ==="
 echo
 
-run_qemu -drive "file=$TEST_IMG.base,if=none,id=drive0" <<EOF
+run_qemu -drive "file=$TEST_IMG.base,format=raw,if=none,id=drive0" <<EOF
 { "execute": "qmp_capabilities" }
 { "execute": "blockdev-add",
     "arguments": {

tests/qemu-iotests/071.out +3 -3

index 5f840a9..f36b898 100644
@@ -12,7 +12,7 @@ read 512/512 bytes at offset 229376
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0
+blkverify: read sector_num=0 nb_sectors=1 contents mismatch in sector 0
 
 === Testing blkverify through file blockref ===

@@ -26,7 +26,7 @@ read 512/512 bytes at offset 229376
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0
+blkverify: read sector_num=0 nb_sectors=1 contents mismatch in sector 0
 
 === Testing blkdebug through filename ===

@@ -61,7 +61,7 @@ blkverify: read sector_num=0 nb_sectors=1 contents mismatch in sector 0
 
 === Testing blkverify on existing raw block device ===
 
-Testing: -drive file=TEST_DIR/t.IMGFMT.base,if=none,id=drive0
+Testing: -drive file=TEST_DIR/t.IMGFMT.base,format=raw,if=none,id=drive0
 QMP_VERSION
 {"return": {}}
 {"return": {}}

tests/qemu-iotests/077 +1 -1

index 4dd1bdd..42a9085 100755
@@ -52,7 +52,7 @@ echo "== Some concurrent requests involving RMW =="
 
 function test_io()
 {
-echo "open -o file.align=4k blkdebug::$TEST_IMG"
+echo "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG"
 # A simple RMW request
 cat  <<EOF
 aio_write -P 10 0x200 0x200

tests/qemu-iotests/081 +6 -2

index ed3c29e..9ab93ff 100755
@@ -59,9 +59,13 @@ function run_qemu()
 test_quorum=$($QEMU_IMG --help|grep quorum)
 [ "$test_quorum" = "" ] && _supported_fmt quorum
 
-quorum="file.driver=quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
+quorum="driver=raw,file.driver=quorum,file.vote-threshold=2"
+quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
 quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
-quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2"
+quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw"
+quorum="$quorum,file.children.0.driver=raw"
+quorum="$quorum,file.children.1.driver=raw"
+quorum="$quorum,file.children.2.driver=raw"
 
 echo
 echo "== creating quorum files =="

tests/qemu-iotests/081.out +1 -1

index 073515e..190905a 100644
@@ -55,5 +55,5 @@ wrote 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == checking that quorum is broken ==
-qemu-io: can't open: Could not read image for determining its format: Input/output error
+read failed: Input/output error
 *** done

tests/qemu-iotests/089 +4 -2

index dffc977..6f74cec 100755
@@ -65,7 +65,8 @@ $QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
 
 $QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
 
-$QEMU_IO -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+$QEMU_IO_PROG --cache $CACHEMODE \
+         -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
          -c 'read -P 66 1024 512' "json:{
     \"driver\": \"$IMGFMT\",
     \"file\": {

@@ -91,7 +92,8 @@ $QEMU_IO -c 'write -P 42 0x38000 512' "$TEST_IMG" | _filter_qemu_io
 
 # The "image.filename" part tests whether "a": { "b": "c" } and "a.b": "c" do
 # the same (which they should).
-$QEMU_IO -c 'read -P 42 0x38000 512' "json:{
+$QEMU_IO_PROG --cache $CACHEMODE \
+     -c 'read -P 42 0x38000 512' "json:{
     \"driver\": \"$IMGFMT\",
     \"file\": {
         \"driver\": \"blkdebug\",

tests/qemu-iotests/common +1 -1

index 9e12bec..b923cd2 100644
@@ -391,7 +391,7 @@ BEGIN        { for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \
 done
 
 # Set qemu-io cache mode with $CACHEMODE we have
-QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --cache $CACHEMODE"
+QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT --cache $CACHEMODE"
 
 # Set default options for qemu-img create -o if they were not specified
 _set_default_imgopts

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #3 lore raw

PATCH v3 3/9 qemu-iotests: Add qemu-io format option in Python tests

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

---
 tests/qemu-iotests/030 | 22 +++++++++++-----------
 tests/qemu-iotests/040 | 32 ++++++++++++++++----------------
 tests/qemu-iotests/055 | 18 +++++++++---------
 3 files changed, 36 insertions(+), 36 deletions(-)

tests/qemu-iotests/030 +11 ~~-11~~

index 8ce2373..952a524 100755
@@ -34,7 +34,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         iotests.create_image(backing_img, TestSingleDrive.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
-        qemu_io('-c', 'write -P 0x1 0 512', backing_img)
+        qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
         self.vm = iotests.VM().add_drive("blkdebug::" + test_img)
         self.vm.launch()

@@ -55,8 +55,8 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-c', 'map', backing_img),
-                         qemu_io('-c', 'map', test_img),
+        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
                          'image file map does not match backing file after streaming')
 
     def test_stream_pause(self):

@@ -86,8 +86,8 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-c', 'map', backing_img),
-                         qemu_io('-c', 'map', test_img),
+        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
                          'image file map does not match backing file after streaming')
 
     def test_stream_partial(self):

@@ -101,8 +101,8 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-c', 'map', mid_img),
-                         qemu_io('-c', 'map', test_img),
+        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
+                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
                          'image file map does not match backing file after streaming')
 
     def test_device_not_found(self):

@@ -359,9 +359,9 @@ class TestStreamStop(iotests.QMPTestCase):
 
     def setUp(self):
         qemu_img('create', backing_img, str(TestStreamStop.image_len))
-        qemu_io('-c', 'write -P 0x1 0 32M', backing_img)
+        qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 32M', backing_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
-        qemu_io('-c', 'write -P 0x1 32M 32M', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 32M 32M', test_img)
         self.vm = iotests.VM().add_drive("blkdebug::" + test_img)
         self.vm.launch()

@@ -388,9 +388,9 @@ class TestSetSpeed(iotests.QMPTestCase):
 
     def setUp(self):
         qemu_img('create', backing_img, str(TestSetSpeed.image_len))
-        qemu_io('-c', 'write -P 0x1 0 32M', backing_img)
+        qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 32M', backing_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
-        qemu_io('-c', 'write -P 0x1 32M 32M', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 32M 32M', test_img)
         self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
         self.vm.launch()

tests/qemu-iotests/040 +16 ~~-16~~

index 2b432ad..ea2f98e 100755
@@ -76,8 +76,8 @@ class TestSingleDrive(ImageCommitTestCase):
         iotests.create_image(backing_img, self.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
-        qemu_io('-c', 'write -P 0xab 0 524288', backing_img)
-        qemu_io('-c', 'write -P 0xef 524288 524288', mid_img)
+        qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()

@@ -89,8 +89,8 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_commit(self):
         self.run_commit_test(mid_img, backing_img)
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img)

@@ -116,13 +116,13 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_top_is_active(self):
         self.run_commit_test(test_img, backing_img, need_ready=True)
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
     def test_top_is_default_active(self):
         self.run_default_commit_test()
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()

@@ -159,8 +159,8 @@ class TestRelativePaths(ImageCommitTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.mid_img_abs, self.test_img)
         qemu_img('rebase', '-u', '-b', self.backing_img, self.mid_img_abs)
         qemu_img('rebase', '-u', '-b', self.mid_img, self.test_img)
-        qemu_io('-c', 'write -P 0xab 0 524288', self.backing_img_abs)
-        qemu_io('-c', 'write -P 0xef 524288 524288', self.mid_img_abs)
+        qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', self.backing_img_abs)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', self.mid_img_abs)
         self.vm = iotests.VM().add_drive(self.test_img)
         self.vm.launch()

@@ -179,8 +179,8 @@ class TestRelativePaths(ImageCommitTestCase):
 
     def test_commit(self):
         self.run_commit_test(self.mid_img, self.backing_img)
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % self.mid_img)

@@ -206,8 +206,8 @@ class TestRelativePaths(ImageCommitTestCase):
 
     def test_top_is_active(self):
         self.run_commit_test(self.test_img, self.backing_img)
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
 
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()

@@ -223,8 +223,8 @@ class TestSetSpeed(ImageCommitTestCase):
         qemu_img('create', backing_img, str(TestSetSpeed.image_len))
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
-        qemu_io('-c', 'write -P 0x1 0 512', test_img)
-        qemu_io('-c', 'write -P 0xef 524288 524288', mid_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 0 512', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()

tests/qemu-iotests/055 +9 -9

index 451b67d..0872444 100755
@@ -34,10 +34,10 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         # Write data to the image so we can compare later
         qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
-        qemu_io('-c', 'write -P0x5d 0 64k', test_img)
-        qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
-        qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
-        qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 0 64k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xd5 1M 32k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xdc 32M 124k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xdc 67043328 64k', test_img)
 
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()

@@ -115,7 +115,7 @@ class TestSetSpeed(iotests.QMPTestCase):
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSetSpeed.image_len))
-        qemu_io('-c', 'write -P1 0 512', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P1 0 512', test_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()

@@ -186,10 +186,10 @@ class TestSingleTransaction(iotests.QMPTestCase):
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleTransaction.image_len))
-        qemu_io('-c', 'write -P0x5d 0 64k', test_img)
-        qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
-        qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
-        qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 0 64k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xd5 1M 32k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xdc 32M 124k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0xdc 67043328 64k', test_img)
 
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #4 lore raw

PATCH v3 4/9 qtests: Specify image format explicitly

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

---
 tests/ahci-test.c         | 3 ++-
 tests/bios-tables-test.c  | 2 +-
 tests/drive_del-test.c    | 2 +-
 tests/fdc-test.c          | 2 +-
 tests/hd-geo-test.c       | 2 +-
 tests/i440fx-test.c       | 5 +++--
 tests/ide-test.c          | 9 +++++----
 tests/nvme-test.c         | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 4 ++--
 tests/virtio-scsi-test.c  | 4 ++--
 12 files changed, 21 insertions(+), 18 deletions(-)

tests/ahci-test.c +2 -1

index 4c77ebe..e77fa3a 100644
@@ -487,7 +487,8 @@ static void qtest_shutdown(void)
  */
 static QPCIDevice *ahci_boot(void)
 {
-    qtest_boot("-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
+    qtest_boot("-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s,"
+               "format=raw"
                " -M q35 "
                "-device ide-hd,drive=drive0 "
                "-global ide-hd.ver=%s",

tests/bios-tables-test.c +1 -1

index 9e4d205..2519f7d 100644
@@ -716,7 +716,7 @@ static void test_acpi_one(const char *params, test_data *data)
     int i;
 
     args = g_strdup_printf("-net none -display none %s "
-                           "-drive id=hd0,if=none,file=%s "
+                           "-drive id=hd0,if=none,file=%s,format=raw "
                            "-device ide-hd,drive=hd0 ",
                            params ? params : "", disk);

tests/drive_del-test.c +1 -1

index 53fa969..8951f6f 100644
@@ -103,7 +103,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
     /* Start with a drive used by a device that unplugs instantaneously */
-    qtest_start("-drive if=none,id=drive0,file=/dev/null"
+    qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
                 " -device virtio-scsi-pci"
                 " -device scsi-hd,drive=drive0,id=dev0");

tests/fdc-test.c +1 -1

index 203074c..3c6c83c 100644
@@ -291,7 +291,7 @@ static void test_media_insert(void)
     /* Insert media in drive. DSKCHK should not be reset until a step pulse
      * is sent. */
     qmp_discard_response("{'execute':'change', 'arguments':{"
-                         " 'device':'floppy0', 'target': %s }}",
+                         " 'device':'floppy0', 'target': %s, 'arg': 'raw' }}",
                          test_image);
     qmp_discard_response(""); /* ignore event
                                  (FIXME open -> open transition?!) */

tests/hd-geo-test.c +1 -1

index c84d1e7..7cc8dff 100644
@@ -208,7 +208,7 @@ static int setup_ide(int argc, char *argv[], int argv_sz,
 {
     char *s1, *s2, *s3;
 
-    s1 = g_strdup_printf("-drive id=drive%d,if=%s",
+    s1 = g_strdup_printf("-drive id=drive%d,if=%s,format=raw",
                          ide_idx, dev ? "none" : "ide");
     s2 = dev ? g_strdup("") : g_strdup_printf(",index=%d", ide_idx);

tests/i440fx-test.c +3 -2

index ad232b5..a3f7279 100644
@@ -342,8 +342,9 @@ static void test_i440fx_firmware(FirmwareTestFixture *fixture,
     g_assert(fw_pathname != NULL);
 
     /* Better hope the user didn't put metacharacters in TMPDIR and co. */
-    cmdline = g_strdup_printf("-S %s %s",
-                              fixture->is_bios ? "-bios" : "-pflash",
+    cmdline = g_strdup_printf("-S %s%s", fixture->is_bios
+                                         ? "-bios "
+                                         : "-drive if=pflash,format=raw,file=",
                               fw_pathname);
     g_test_message("qemu cmdline: %s", cmdline);
     qtest_start(cmdline);

tests/ide-test.c +5 -4

index b7a97e9..29f4039 100644
@@ -385,7 +385,7 @@ static void test_bmdma_no_busmaster(void)
 static void test_bmdma_setup(void)
 {
     ide_test_start(
-        "-drive file=%s,if=ide,serial=%s,cache=writeback "
+        "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
         "-global ide-hd.ver=%s",
         tmp_path, "testdisk", "version");
 }

@@ -414,7 +414,7 @@ static void test_identify(void)
     int ret;
 
     ide_test_start(
-        "-drive file=%s,if=ide,serial=%s,cache=writeback "
+        "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
         "-global ide-hd.ver=%s",
         tmp_path, "testdisk", "version");

@@ -458,7 +458,7 @@ static void test_flush(void)
     uint8_t data;
 
     ide_test_start(
-        "-drive file=blkdebug::%s,if=ide,cache=writeback",
+        "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
         tmp_path);
 
     /* Delay the completion of the flush request until we explicitly do it */

@@ -526,7 +526,8 @@ static void test_retry_flush(void)
 
     ide_test_start(
         "-vnc none "
-        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
+        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,format=raw,"
+        "rerror=stop,werror=stop",
         debug_path, tmp_path);
 
     /* FLUSH CACHE command on device 0*/

tests/nvme-test.c +1 -1

index 85768e8..ff38b5e 100644
@@ -24,7 +24,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/nvme/nop", nop);
 
-    qtest_start("-drive id=drv0,if=none,file=/dev/null "
+    qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
                 "-device nvme,drive=drv0,serial=foo");
     ret = g_test_run();

tests/usb-hcd-uhci-test.c +1 -1

index 8cf2c5b..6d631cf 100644
@@ -87,7 +87,7 @@ int main(int argc, char **argv)
     qtest_add_func("/uhci/pci/hotplug/usb-storage", test_usb_storage_hotplug);
 
     qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
-                " -drive id=drive0,if=none,file=/dev/null"
+                " -drive id=drive0,if=none,file=/dev/null,format=raw"
                 " -device usb-tablet,bus=uhci.0,port=1");
     ret = g_test_run();
     qtest_end();

tests/usb-hcd-xhci-test.c +1 -1

index b1a7dec..adf261e 100644
@@ -91,7 +91,7 @@ int main(int argc, char **argv)
     qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
     qtest_start("-device nec-usb-xhci,id=xhci"
-                " -drive id=drive0,if=none,file=/dev/null");
+                " -drive id=drive0,if=none,file=/dev/null,format=raw");
     ret = g_test_run();
     qtest_end();

tests/virtio-blk-test.c +2 -2

index ead3911..89d7cbf 100644
@@ -68,8 +68,8 @@ static QPCIBus *test_start(void)
     g_assert_cmpint(ret, ==, 0);
     close(fd);
 
-    cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s "
-                              "-drive if=none,id=drive1,file=/dev/null "
+    cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s,format=raw "
+                              "-drive if=none,id=drive1,file=/dev/null,format=raw "
                               "-device virtio-blk-pci,id=drv0,drive=drive0,"
                               "addr=%x.%x",
                               tmp_path, PCI_SLOT, PCI_FN);

tests/virtio-scsi-test.c +2 -2

index 41f9602..989f825 100644
@@ -52,8 +52,8 @@ int main(int argc, char **argv)
     qtest_add_func("/virtio/scsi/pci/nop", pci_nop);
     qtest_add_func("/virtio/scsi/pci/hotplug", hotplug);
 
-    qtest_start("-drive id=drv0,if=none,file=/dev/null "
-                "-drive id=drv1,if=none,file=/dev/null "
+    qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+                "-drive id=drv1,if=none,file=/dev/null,format=raw "
                 "-device virtio-scsi-pci,id=vscsi0 "
                 "-device scsi-hd,bus=vscsi0.0,drive=drv0");
     ret = g_test_run();

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #5 lore raw

PATCH v3 5/9 block: Factor bdrv_probe_all() out of find_image_format()

From: Markus Armbruster <[email protected]>

Signed-off-by Markus Armbruster <[email protected]>

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

---
 block.c | 48 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

block.c +35 ~~-13~~

index a612594..a0051cb 100644
@@ -648,11 +648,43 @@ BlockDriver *bdrv_find_protocol(const char *filename,
     return NULL;
 }
 
+/*
+ * Guess image format by probing its contents.
+ * This is not a good idea when your image is raw (CVE-2008-2004), but
+ * we do it anyway for backward compatibility.
+ *
+ * @buf         contains the image's first @buf_size bytes.
+ * @buf_size    is the buffer size in bytes (generally 2048, but can be smaller
+ *              if the image file is smaller)
+ * @filename    is its filename.
+ *
+ * For all block drivers, call the bdrv_probe() method to get its
+ * probing score.
+ * Return the first block driver with the highest probing score.
+ */
+static BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
+                                   const char *filename)
+{
+    int score_max = 0, score;
+    BlockDriver *drv = NULL, *d;
+
+    QLIST_FOREACH(d, &bdrv_drivers, list) {
+        if (d->bdrv_probe) {
+            score = d->bdrv_probe(buf, buf_size, filename);
+            if (score > score_max) {
+                score_max = score;
+                drv = d;
+            }
+        }
+    }
+
+    return drv;
+}
+
 static int find_image_format(BlockDriverState *bs, const char *filename,
                              BlockDriver **pdrv, Error **errp)
 {
-    int score, score_max;
-    BlockDriver *drv1, *drv;
+    BlockDriver *drv;
     uint8_t buf[2048];
     int ret = 0;

@@ -675,17 +707,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
         return ret;
     }
 
-    score_max = 0;
-    drv = NULL;
-    QLIST_FOREACH(drv1, &bdrv_drivers, list) {
-        if (drv1->bdrv_probe) {
-            score = drv1->bdrv_probe(buf, ret, filename);
-            if (score > score_max) {
-                score_max = score;
-                drv = drv1;
-            }
-        }
-    }
+    drv = bdrv_probe_all(buf, ret, filename);
     if (!drv) {
         error_setg(errp, "Could not determine image format: No compatible "
                    "driver found");

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #6 lore raw

PATCH v3 6/9 block: Read only one sector for format probing

The only image format driver that even potentially accesses anything after 512 bytes in its bdrv_probe() implementation is VMDK, which reads a plain-text descriptor file. In practice, the field it's looking for seems to come first and will be well within the first 512 bytes, too.

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

---
 block.c                   | 6 +++---
 include/block/block_int.h | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

block.c +3 -3

index a0051cb..9114cfa 100644
@@ -654,8 +654,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
  * we do it anyway for backward compatibility.
  *
  * @buf         contains the image's first @buf_size bytes.
- * @buf_size    is the buffer size in bytes (generally 2048, but can be smaller
- *              if the image file is smaller)
+ * @buf_size    is the buffer size in bytes (generally BLOCK_PROBE_BUF_SIZE,
+ *              but can be smaller if the image file is smaller)
  * @filename    is its filename.
  *
  * For all block drivers, call the bdrv_probe() method to get its

@@ -685,7 +685,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
                              BlockDriver **pdrv, Error **errp)
 {
     BlockDriver *drv;
-    uint8_t buf[2048];
+    uint8_t buf[BLOCK_PROBE_BUF_SIZE];
     int ret = 0;
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */

include/block/block_int.h +2 -0

index a1c17b9..cd94559 100644
@@ -57,6 +57,8 @@
 #define BLOCK_OPT_REDUNDANCY        "redundancy"
 #define BLOCK_OPT_NOCOW             "nocow"
 
+#define BLOCK_PROBE_BUF_SIZE        512
+
 typedef struct BdrvTrackedRequest {
     BlockDriverState *bs;
     int64_t offset;

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #7 lore raw

PATCH v3 7/9 raw: Prohibit dangerous writes for probed images

If the user neglects to specify the image format, QEMU probes the image to guess it automatically, for convenience.

Relying on format probing is insecure for raw images (CVE-2008-2004). If the guest writes a suitable header to the device, the next probe will recognize a format chosen by the guest. A malicious guest can abuse this to gain access to host files, e.g. by crafting a QCOW2 header with backing file /etc/shadow.

Commit 1e72d3b (April 2008) provided -drive parameter format to let users disable probing. Commit f965509 (March 2009) extended QCOW2 to optionally store the backing file format, to let users disable backing file probing. QED has had a flag to suppress probing since the beginning (2010), set whenever a raw backing file is assigned.

All of these additions that allow to avoid format probing have to be specified explicitly. The default still allows the attack.

In order to fix this, commit 79368c8 (July 2010) put probed raw images in a restricted mode, in which they wouldn't be able to overwrite the first few bytes of the image so that they would identify as a different image. If a write to the first sector would write one of the signatures of another driver, qemu would instead zero out the first four bytes. This patch was later reverted in commit 8b33d9e (September 2010) because it didn't get the handling of unaligned qiov members right.

Today's block layer that is based on coroutines and has qiov utility functions makes it much easier to get this functionality right, so this patch implements it.

The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).

Note that this change doesn't introduce new breakage for false positive cases where the guest legitimately writes data into the first sector that matches the signatures of an image format (e.g. for nested virt):
These cases were broken before, only the failure mode changes from corruption after the next restart (when the wrong format is probed) to failing the problematic write request.

Also note that like in the original patch, the restrictions only apply if the image format has been guessed by probing. Explicitly specifying a format allows guests to write anything they like.

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

Reviewed-by Eric Blake <[email protected]>

---
 block.c                   |  5 ++--
 block/raw_bsd.c           | 64 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |  3 +++
 3 files changed, 69 insertions(+), 3 deletions(-)

block.c +3 -2

index 9114cfa..e4aa53d 100644
@@ -662,8 +662,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
  * probing score.
  * Return the first block driver with the highest probing score.
  */
-static BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
-                                   const char *filename)
+BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
+                            const char *filename)
 {
     int score_max = 0, score;
     BlockDriver *drv = NULL, *d;

@@ -1489,6 +1489,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Image format probing */
+    bs->probed = !drv;
     if (!drv && file) {
         ret = find_image_format(file, filename, &drv, &local_err);
         if (ret < 0) {

block/raw_bsd.c +63 -1

77 lines not on this page · raw message

include/block/block_int.h +3 -0

index cd94559..192fce8 100644
@@ -326,6 +326,7 @@ struct BlockDriverState {
     int sg;        /* if true, the device is a /dev/sg* */
     int copy_on_read; /* if true, copy read backing sectors into image
                          note this is a reference count */
+    bool probed;
 
     BlockDriver *drv; /* NULL means no media */
     void *opaque;

@@ -414,6 +415,8 @@ struct BlockDriverState {
 };
 
 int get_tmp_filename(char *filename, int size);
+BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
+                            const char *filename);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
                         ThrottleConfig *cfg);

Signature · 1 line

1.8.3.1

Dr. David Alan Gilbert

2014-11-20 20:09 UTC #8 lore raw

Quoted 32 lines · * Kevin Wolf ([email protected]) wrote:

block/raw_bsd.c +24 -0

index 401b967..2ce5409 100644
@@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    void *buf = NULL;
+    BlockDriver *drv;
+    QEMUIOVector local_qiov;
+    int ret;
+
+    if (bs->probed && sector_num == 0) {
+        /* As long as these conditions are true, we can't get partial writes to
+         * the probe buffer and can just directly check the request. */
+        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
+        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
+
+        if (nb_sectors == 0) {
+            /* qemu_iovec_to_buf() would fail, but we want to return success
+             * instead of -EINVAL in this case. */
+            return 0;
+        }
+
+        buf = qemu_try_blockalign(bs->file, 512);
+        if (!buf) {
+            ret = -ENOMEM;
+            goto fail;
+        }
+
+        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);

block/raw_bsd.c static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, · line 94

Quoted 10 lines

block/raw_bsd.c +10 -0

+        if (ret != 512) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        drv = bdrv_probe_all(buf, 512, NULL);
+        if (drv != bs->drv) {
+            ret = -EPERM;
+            goto fail;
+        }

Two things about this worry me:
1) It allows a running guest to prod at the probing code potentially quite
hard; if there is anything nasty that can be done during probing it would potentially make it easier for a guest to find it.

2) We don't log anything when this failure happens so if someone hits
this by accident for some reason it'll confuse them no end. Could we add a (1 time?) error_report/printf just so that there's something to work with ?

Dave

Quoted 66 lines

block/raw_bsd.c +29 -1

+
+        /* Use the checked buffer, a malicious guest might be overwriting its
+         * original buffer in the background. */
+        qemu_iovec_init(&local_qiov, qiov->niov + 1);
+        qemu_iovec_add(&local_qiov, buf, 512);
+        qemu_iovec_concat(&local_qiov, qiov, 512, qiov->size - 512);
+        qiov = &local_qiov;
+    }
+
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
-    return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
+    ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
+
+fail:
+    if (qiov == &local_qiov) {
+        qemu_iovec_destroy(&local_qiov);
+    }
+    qemu_vfree(buf);
+    return ret;
 }
 
 static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,

@@ -158,6 +208,18 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     bs->sg = bs->file->sg;
+
+    if (bs->probed && !bdrv_is_read_only(bs)) {
+        fprintf(stderr,
+                "WARNING: Image format was not specified for '%s' and probing "
+                "guessed raw.\n"
+                "         Automatically detecting the format is dangerous for "
+                "raw images, write operations on block 0 will be restricted.\n"
+                "         Specify the 'raw' format explicitly to remove the "
+                "restrictions.\n",
+                bs->file->filename);
+    }
+
     return 0;
 }

include/block/block_int.h +3 -0

index cd94559..192fce8 100644
@@ -326,6 +326,7 @@ struct BlockDriverState {
     int sg;        /* if true, the device is a /dev/sg* */
     int copy_on_read; /* if true, copy read backing sectors into image
                          note this is a reference count */
+    bool probed;
 
     BlockDriver *drv; /* NULL means no media */
     void *opaque;

@@ -414,6 +415,8 @@ struct BlockDriverState {
 };
 
 int get_tmp_filename(char *filename, int size);
+BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
+                            const char *filename);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
                         ThrottleConfig *cfg);

Signature · 1 line

1.8.3.1

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Kevin Wolf

author

2014-11-21 10:16 UTC #9 lore raw

Am 20.11.2014 um 21:08 hat Dr. David Alan Gilbert geschrieben:

Quoted 42 lines · * Kevin Wolf ([email protected]) wrote:

Quoted 41 lines

block/raw_bsd.c +31 -0

index 401b967..2ce5409 100644
@@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    void *buf = NULL;
+    BlockDriver *drv;
+    QEMUIOVector local_qiov;
+    int ret;
+
+    if (bs->probed && sector_num == 0) {
+        /* As long as these conditions are true, we can't get partial writes to
+         * the probe buffer and can just directly check the request. */
+        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
+        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
+
+        if (nb_sectors == 0) {
+            /* qemu_iovec_to_buf() would fail, but we want to return success
+             * instead of -EINVAL in this case. */
+            return 0;
+        }
+
+        buf = qemu_try_blockalign(bs->file, 512);
+        if (!buf) {
+            ret = -ENOMEM;
+            goto fail;
+        }
+
+        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
+        if (ret != 512) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        drv = bdrv_probe_all(buf, 512, NULL);
+        if (drv != bs->drv) {

Quoted 8 lines

block/raw_bsd.c static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, · line 94
Quoted 3 lines
block/raw_bsd.c +3 -0
+            ret = -EPERM;
+            goto fail;
+        }
Two things about this worry me:
1) It allows a running guest to prod at the probing code potentially quite
hard; if there is anything nasty that can be done during probing it would potentially make it easier for a guest to find it.

The probing functions are trivial. You can audit them in no time even with no previous block layer experience. They just do a few tests on the passed buffer.

Quoted 3 lines

2) We don't log anything when this failure happens so if someone hits
this by accident for some reason it'll confuse them no end. Could we add a (1 time?) error_report/printf just so that there's something to work with ?

We already log a warning on bdrv_open(). Don't you think that should be enough?

Kevin

Dr. David Alan Gilbert

2014-11-21 10:27 UTC #10 lore raw

Quoted 52 lines · * Kevin Wolf ([email protected]) wrote:

Am 20.11.2014 um 21:08 hat Dr. David Alan Gilbert geschrieben:

Quoted 42 lines · * Kevin Wolf ([email protected]) wrote:

Quoted 41 lines

block/raw_bsd.c +31 -0

index 401b967..2ce5409 100644
@@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    void *buf = NULL;
+    BlockDriver *drv;
+    QEMUIOVector local_qiov;
+    int ret;
+
+    if (bs->probed && sector_num == 0) {
+        /* As long as these conditions are true, we can't get partial writes to
+         * the probe buffer and can just directly check the request. */
+        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
+        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
+
+        if (nb_sectors == 0) {
+            /* qemu_iovec_to_buf() would fail, but we want to return success
+             * instead of -EINVAL in this case. */
+            return 0;
+        }
+
+        buf = qemu_try_blockalign(bs->file, 512);
+        if (!buf) {
+            ret = -ENOMEM;
+            goto fail;
+        }
+
+        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
+        if (ret != 512) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        drv = bdrv_probe_all(buf, 512, NULL);
+        if (drv != bs->drv) {

Quoted 8 lines

block/raw_bsd.c static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, · line 94
Quoted 3 lines
block/raw_bsd.c +3 -0
+            ret = -EPERM;
+            goto fail;
+        }
Two things about this worry me:
1) It allows a running guest to prod at the probing code potentially quite
hard; if there is anything nasty that can be done during probing it would potentially make it easier for a guest to find it.

Quoted 3 lines

The probing functions are trivial. You can audit them in no time even with no previous block layer experience. They just do a few tests on the passed buffer.

Hmm, OK.

Quoted 6 lines

Quoted 3 lines
2) We don't log anything when this failure happens so if someone hits
this by accident for some reason it'll confuse them no end. Could we add a (1 time?) error_report/printf just so that there's something to work with ?
We already log a warning on bdrv_open(). Don't you think that should be enough?

Imagine a place that's got lots of QEMUs running already, they'll install the QEMU that has this fix in it and carry on; they won't look at their logs. Then if they hit a problem with a write failure they'll get confused; the logs in bdrv_open just tell them they should be doing something safely, they don't tell them they've actually hit a case fo something trying to do the write.

Dave

Quoted 1 line

Kevin

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Stefan Hajnoczi

Reviewed-by on v3 7/9

2014-11-25 16:52 UTC #11 lore raw

Quoted 47 lines · Kevin Wolf

If the user neglects to specify the image format, QEMU probes the image to guess it automatically, for convenience.
Relying on format probing is insecure for raw images (CVE-2008-2004). If the guest writes a suitable header to the device, the next probe will recognize a format chosen by the guest. A malicious guest can abuse this to gain access to host files, e.g. by crafting a QCOW2 header with backing file /etc/shadow.
Commit 1e72d3b (April 2008) provided -drive parameter format to let users disable probing. Commit f965509 (March 2009) extended QCOW2 to optionally store the backing file format, to let users disable backing file probing. QED has had a flag to suppress probing since the beginning (2010), set whenever a raw backing file is assigned.
All of these additions that allow to avoid format probing have to be specified explicitly. The default still allows the attack.
In order to fix this, commit 79368c8 (July 2010) put probed raw images in a restricted mode, in which they wouldn't be able to overwrite the first few bytes of the image so that they would identify as a different image. If a write to the first sector would write one of the signatures of another driver, qemu would instead zero out the first four bytes. This patch was later reverted in commit 8b33d9e (September 2010) because it didn't get the handling of unaligned qiov members right.
Today's block layer that is based on coroutines and has qiov utility functions makes it much easier to get this functionality right, so this patch implements it.
The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).
Note that this change doesn't introduce new breakage for false positive cases where the guest legitimately writes data into the first sector that matches the signatures of an image format (e.g. for nested virt):
These cases were broken before, only the failure mode changes from corruption after the next restart (when the wrong format is probed) to failing the problematic write request.
Also note that like in the original patch, the restrictions only apply if the image format has been guessed by probing. Explicitly specifying a format allows guests to write anything they like.

Quoted 8 lines

Signed-off-by Kevin Wolf <[email protected]>
Reviewed-by Eric Blake <[email protected]>
Reviewed-by Max Reitz <[email protected]>
---
 block.c                   |  5 ++--
 block/raw_bsd.c           | 64 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |  3 +++
 3 files changed, 69 insertions(+), 3 deletions(-)

Kevin Wolf

author

2014-11-20 15:27 UTC #12 lore raw

PATCH v3 8/9 qemu-iotests: Fix stderr handling in common.qemu

The original intention was to pipe stderr of qemu into $fifo_out. However, the redirections were specified in the wrong order for this. This patch fixes it.

Now qemu's output on stderr can be retrieved with _send_qemu_cmd, which applies several useful filters on the output that were missing before.

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

Reviewed-by Eric Blake <[email protected]>

---
 tests/qemu-iotests/common.qemu | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/qemu-iotests/common.qemu +2 -1

index ee7ebb4..8e618b5 100644
@@ -153,8 +153,9 @@ function _launch_qemu()
     mkfifo "${fifo_out}"
     mkfifo "${fifo_in}"
 
-    "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" 2>&1 \
+    "${QEMU}" -nographic -serial none ${comm} -machine accel=qtest "${@}" \
                                                                 >"${fifo_out}" \
+                                                                2>&1 \
                                                                 <"${fifo_in}" &
     QEMU_PID[${_QEMU_HANDLE}]=$!

Signature · 1 line

1.8.3.1

Kevin Wolf

author

2014-11-20 15:27 UTC #13 lore raw

PATCH v3 9/9 qemu-iotests: Test writing non-raw image headers to raw image

This is forbidden if the raw driver was probed.

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

---
 tests/qemu-iotests/109                            | 132 +++++++++++++
 tests/qemu-iotests/109.out                        | 231 ++++++++++++++++++++++
 tests/qemu-iotests/group                          |   1 +
 tests/qemu-iotests/sample_images/grub_mbr.raw.bz2 | Bin 0 -> 552 bytes
 4 files changed, 364 insertions(+)
 create mode 100755 tests/qemu-iotests/109
 create mode 100644 tests/qemu-iotests/109.out
 create mode 100644 tests/qemu-iotests/sample_images/grub_mbr.raw.bz2

tests/qemu-iotests/109 new file +132 -0

new file mode 100755
index 0000000..0b668da
@@ -0,0 +1,132 @@
+#!/bin/bash
+#
+# Test writing image headers of other formats into raw images
+#
+# 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.src
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+qemu_comm_method=qmp
+
+function run_qemu()
+{
+    local raw_img="$1"
+    local source_img="$2"
+    local qmp_format="$3"
+    local qmp_event="$4"
+
+    _launch_qemu -drive file="${source_img}",format=raw,cache=${CACHEMODE},id=src
+    _send_qemu_cmd $QEMU_HANDLE "{ 'execute': 'qmp_capabilities' }" "return"
+
+    _send_qemu_cmd $QEMU_HANDLE \
+        "{'execute':'drive-mirror', 'arguments':{
+            'device': 'src', 'target': '$raw_img', $qmp_format
+            'mode': 'existing', 'sync': 'full'}}" \
+        "return"
+
+    _send_qemu_cmd $QEMU_HANDLE '' "$qmp_event"
+    _send_qemu_cmd $QEMU_HANDLE '{"execute":"query-block-jobs"}' "return"
+    _cleanup_qemu
+}
+
+for fmt in qcow qcow2 qed vdi vmdk vpc; do
+
+    echo
+    echo "=== Writing a $fmt header into raw ==="
+    echo
+
+    _make_test_img 64M
+    TEST_IMG="$TEST_IMG.src" IMGFMT=$fmt _make_test_img 64M
+
+    # This first test should fail: The image format was probed, we may not
+    # write an image header at the start of the image
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR"
+    $QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+
+    # When raw was explicitly specified, the same must succeed
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "'format': 'raw'," "BLOCK_JOB_READY"
+    $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src"
+
+done
+
+
+for sample_img in empty.bochs iotest-dirtylog-10G-4M.vhdx parallels-v1 \
+                  simple-pattern.cloop; do
+
+    echo
+    echo "=== Copying sample image $sample_img into raw ==="
+    echo
+
+    # Can't use _use_sample_img because that isn't designed to be used multiple
+    # times and it overwrites $TEST_IMG (both breaks cleanup)
+    _make_test_img 64M
+    bzcat "$SAMPLE_IMG_DIR/$sample_img.bz2" > "$TEST_IMG.src"
+
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR"
+    $QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "'format': 'raw'," "BLOCK_JOB_READY"
+    # qemu-img compare can't handle unaligned file sizes
+    $QEMU_IMG resize -f raw "$TEST_IMG.src" +0
+    $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src"
+done
+
+echo
+echo "=== Write legitimate MBR into raw ==="
+echo
+
+for sample_img in grub_mbr.raw; do
+    _make_test_img 64M
+    bzcat "$SAMPLE_IMG_DIR/$sample_img.bz2" > "$TEST_IMG.src"
+
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_READY"
+    $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src"
+
+    run_qemu "$TEST_IMG" "$TEST_IMG.src" "'format': 'raw'," "BLOCK_JOB_READY"
+    $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src"
+done
+
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0

tests/qemu-iotests/109.out new file +231 -0

231 lines not on this page · raw message

tests/qemu-iotests/group +1 -0

index 7dfe469..af58a68 100644
@@ -111,4 +111,5 @@
 105 rw auto quick
 107 rw auto quick
 108 rw auto quick
+109 rw auto
 111 rw auto quick

tests/qemu-iotests/sample_images/grub_mbr.raw.bz2 new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..8585878bcdec2fdec18a3f31b360fa02450f9ff8
GIT binary patch

Signature · 1 line

1.8.3.1

Max Reitz

Reviewed-by on v3 9/9

2014-11-20 16:18 UTC #14 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] qemu-iotests: Test writing non-raw image headers to raw image

Quoted 9 lines · Kevin Wolf

This is forbidden if the raw driver was probed.

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

---
  tests/qemu-iotests/109                            | 132 +++++++++++++
  tests/qemu-iotests/109.out                        | 231 ++++++++++++++++++++++
  tests/qemu-iotests/group                          |   1 +
  tests/qemu-iotests/sample_images/grub_mbr.raw.bz2 | Bin 0 -> 552 bytes
  4 files changed, 364 insertions(+)

Quoted 3 lines

create mode 100755 tests/qemu-iotests/109
create mode 100644 tests/qemu-iotests/109.out
create mode 100644 tests/qemu-iotests/sample_images/grub_mbr.raw.bz2

Looking at the hexdump of the grub_mbr.raw, it looks like GRUB 2 to me (when comparing it to my MBR and a GRUB Legacy boot sector; I wasn't so sure whether you might go for GRUB Legacy as a late protest against GRUB 2). Good. :-)

Kevin Wolf

author

2014-11-20 16:29 UTC #15 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] qemu-iotests: Test writing non-raw image headers to raw image

Am 20.11.2014 um 17:18 hat Max Reitz geschrieben:

Quoted 14 lines · Kevin Wolf

Quoted 12 lines

This is forbidden if the raw driver was probed.

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

---
 tests/qemu-iotests/109                            | 132 +++++++++++++
 tests/qemu-iotests/109.out                        | 231 ++++++++++++++++++++++
 tests/qemu-iotests/group                          |   1 +
 tests/qemu-iotests/sample_images/grub_mbr.raw.bz2 | Bin 0 -> 552 bytes
 4 files changed, 364 insertions(+)
 create mode 100755 tests/qemu-iotests/109
 create mode 100644 tests/qemu-iotests/109.out
 create mode 100644 tests/qemu-iotests/sample_images/grub_mbr.raw.bz2

Quoted 4 lines

Looking at the hexdump of the grub_mbr.raw, it looks like GRUB 2 to me (when comparing it to my MBR and a GRUB Legacy boot sector; I wasn't so sure whether you might go for GRUB Legacy as a late protest against GRUB 2). Good. :-)

I don't care about the bad usability of a bootloader when I only put some part of it in an MBR image that won't boot anyway. ;-)

My only form of protest is that if I nowadays want to write a bootloader to a disk image from the host, it's syslinux and not GRUB any more. It's not quite as nice, but at least installing that one is possible without damage to your sanity...

Kevin

Stefan Hajnoczi

Applied

2014-11-26 16:24 UTC #16 lore raw