[Qemu-devel] [PATCH 0/2] vmdk: Fix possible segfault with non-VMDK backing

[Qemu-devel] [PATCH 0/2] vmdk: Fix possible segfault with non-VMDK backing

[Max Reitz]

The VMDK driver assumes that its backing file is always a VMDK file,
too, because it verifies that when creating the overlay.  However, that
verification means nothing at runtime, and consequently you can assign
non-VMDK backing files to a VMDK node.  This then breaks because the
driver accesses the backing node as a VMDK node to read its supposed CID
to compare it with the overlay's parentCID entry -- which usually fails,
either in a benign way (we read from a garbage offset, and then we read
garbage or get a read error straight away), or we get a segfault
(because the backing node does not have a respective file child).

Anyway, we just shouldn't do it and instead check whether the backing
file is a VMDK node before treating it like one.


(This fixes
 http://lists.nongnu.org/archive/html/qemu-block/2018-06/msg01268.html)


Max Reitz (2):
  vmdk: Fix possible segfault with non-VMDK backing
  iotests: Add VMDK backing file correlation test

 block/vmdk.c               |   6 ++
 tests/qemu-iotests/225     | 132 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/225.out |  24 +++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 163 insertions(+)
 create mode 100755 tests/qemu-iotests/225
 create mode 100644 tests/qemu-iotests/225.out

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] vmdk: Fix possible segfault with non-VMDK backing

[Max Reitz]

VMDK performs a probing check in vmdk_co_create_opts() to prevent the
user from assigning non-VMDK files as a backing file, because it only
supports VMDK backing files.  However, with the @backing runtime option,
it is possible to assign arbitrary nodes as backing nodes, regardless of
what the image header says.  Therefore, VMDK may not just access backing
nodes assuming they are VMDK nodes -- which it does, because it needs to
compare the backing file's CID with the overlay's parentCID value, and
naturally the backing file only has a CID when it's a VMDK file.
Instead, it should report the CID of non-VMDK backing files not to match
the overlay because clearly a non-present CID does not match.

Without this change, vmdk_read_cid() reads from the backing file's
bs->file, which may be NULL (in which case we get a segfault).  Also, it
interprets bs->opaque as a BDRVVmdkState and then reads from the
.desc_offset field, which usually will just return some arbitrary value
which then results in either garbage to be read, or bdrv_pread() to
return an error, both of which result in a non-matching CID to be
reported.

(In a very unlikely case, we could read something that looks like a
VMDK descriptor, and then get a CID which might actually match.  But
that is highly unlikely, and the only result would be that VMDK accepts
the backing file which is not too bad (albeit unintentional).)

((And in theory, the seek to .desc_offset might leak data from another
block driver's opaque object.  But then again, the user should realize
very quickly that a non-VMDK backing file does not work (because the
read will very likely fail, due to the reasons given above), so this
should not be exploitable.))

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index 84f8bbe480..a9d0084e36 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -333,6 +333,12 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     if (!s->cid_checked && bs->backing) {
         BlockDriverState *p_bs = bs->backing->bs;
 
+        if (strcmp(p_bs->drv->format_name, "vmdk")) {
+            /* Backing file is not in vmdk format, so it does not have
+             * a CID, which makes the overlay's parent CID invalid */
+            return 0;
+        }
+
         if (vmdk_read_cid(p_bs, 0, &cur_pcid) != 0) {
             /* read failure: report as not valid */
             return 0;
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] iotests: Add VMDK backing file correlation test

[Max Reitz]

This new test verifies that VMDK backing file reads fail when the
backing file has a non-matching CID.  This includes non-VMDK backing
files.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/225     | 132 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/225.out |  24 +++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 157 insertions(+)
 create mode 100755 tests/qemu-iotests/225
 create mode 100644 tests/qemu-iotests/225.out

diff --git a/tests/qemu-iotests/225 b/tests/qemu-iotests/225
new file mode 100755
index 0000000000..f2ee715685
--- /dev/null
+++ b/tests/qemu-iotests/225
@@ -0,0 +1,132 @@
+#!/bin/bash
+#
+# Test vmdk backing file correlation
+#
+# Copyright (C) 2018 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
+owner=mreitz@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+here=$PWD
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.not_base"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+# This tests vmdk-specific low-level functionality
+_supported_fmt vmdk
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts "subformat=monolithicFlat" \
+                     "subformat=twoGbMaxExtentFlat" \
+                     "subformat=twoGbMaxExtentSparse"
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 1M
+TEST_IMG="$TEST_IMG.not_base" _make_test_img 1M
+_make_test_img -b "$TEST_IMG.base"
+
+make_opts()
+{
+    node_name=$1
+    filename=$2
+    backing=$3
+
+    if [ -z "$backing" ]; then
+        backing="null"
+    else
+        backing="'$backing'"
+    fi
+
+    echo "{ 'node-name': '$node_name',
+            'driver': 'vmdk',
+            'file': {
+                'driver': 'file',
+                'filename': '$filename'
+            },
+            'backing': $backing }"
+}
+
+overlay_opts=$(make_opts overlay "$TEST_IMG" backing)
+base_opts=$(make_opts backing "$TEST_IMG.base")
+not_base_opts=$(make_opts backing "$TEST_IMG.not_base")
+
+not_vmdk_opts="{ 'node-name': 'backing', 'driver': 'null-co' }"
+
+echo
+echo '=== Testing fitting VMDK backing image ==='
+echo
+
+qemu_comm_method=monitor \
+    _launch_qemu -blockdev "$base_opts" -blockdev "$overlay_opts"
+
+# Should not return an error
+_send_qemu_cmd $QEMU_HANDLE 'qemu-io overlay "read 0 512"' 'ops'
+
+_cleanup_qemu
+
+
+echo
+echo '=== Testing unrelated VMDK backing image ==='
+echo
+
+qemu_comm_method=monitor \
+    _launch_qemu -blockdev "$not_base_opts" -blockdev "$overlay_opts"
+
+# Should fail (gracefully)
+_send_qemu_cmd $QEMU_HANDLE 'qemu-io overlay "read 0 512"' 'failed'
+
+_cleanup_qemu
+
+
+echo
+echo '=== Testing non-VMDK backing image ==='
+echo
+
+# FIXME: This is the reason why we have to use two -blockdev
+# invocations.  You can only fully override the backing file options
+# if you either specify a node reference (as done here) or the new
+# options contain file.filename (which in this case they do not).
+# In other cases, file.filename will be set to whatever the image
+# header of the overlay contains (which we do not want).  I consider
+# this a FIXME because with -blockdev, you cannot specify "partial"
+# options, so setting file.filename but leaving the rest as specified
+# by the user does not make sense.
+qemu_comm_method=monitor \
+    _launch_qemu -blockdev "$not_vmdk_opts" -blockdev "$overlay_opts"
+
+# Should fail (gracefully)
+_send_qemu_cmd $QEMU_HANDLE 'qemu-io overlay "read 0 512"' 'failed'
+
+_cleanup_qemu
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/225.out b/tests/qemu-iotests/225.out
new file mode 100644
index 0000000000..4dc8ee282f
--- /dev/null
+++ b/tests/qemu-iotests/225.out
@@ -0,0 +1,24 @@
+QA output created by 225
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT.not_base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base
+
+=== Testing fitting VMDK backing image ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io overlay "read 0 512"
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing unrelated VMDK backing image ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io overlay "read 0 512"
+read failed: Invalid argument
+
+=== Testing non-VMDK backing image ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io overlay "read 0 512"
+read failed: Invalid argument
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index eea75819d2..a6eedfe727 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -220,3 +220,4 @@
 218 rw auto quick
 219 rw auto
 221 rw auto quick
+225 rw auto quick
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] vmdk: Fix possible segfault with non-VMDK backing

[Fam Zheng]

On Mon, 07/02 23:07, Max Reitz wrote:
> VMDK performs a probing check in vmdk_co_create_opts() to prevent the
> user from assigning non-VMDK files as a backing file, because it only
> supports VMDK backing files.  However, with the @backing runtime option,
> it is possible to assign arbitrary nodes as backing nodes, regardless of
> what the image header says.  Therefore, VMDK may not just access backing
> nodes assuming they are VMDK nodes -- which it does, because it needs to
> compare the backing file's CID with the overlay's parentCID value, and
> naturally the backing file only has a CID when it's a VMDK file.
> Instead, it should report the CID of non-VMDK backing files not to match
> the overlay because clearly a non-present CID does not match.
> 
> Without this change, vmdk_read_cid() reads from the backing file's
> bs->file, which may be NULL (in which case we get a segfault).  Also, it
> interprets bs->opaque as a BDRVVmdkState and then reads from the
> .desc_offset field, which usually will just return some arbitrary value
> which then results in either garbage to be read, or bdrv_pread() to
> return an error, both of which result in a non-matching CID to be
> reported.
> 
> (In a very unlikely case, we could read something that looks like a
> VMDK descriptor, and then get a CID which might actually match.  But
> that is highly unlikely, and the only result would be that VMDK accepts
> the backing file which is not too bad (albeit unintentional).)
> 
> ((And in theory, the seek to .desc_offset might leak data from another
> block driver's opaque object.  But then again, the user should realize
> very quickly that a non-VMDK backing file does not work (because the
> read will very likely fail, due to the reasons given above), so this
> should not be exploitable.))
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 84f8bbe480..a9d0084e36 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -333,6 +333,12 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
>      if (!s->cid_checked && bs->backing) {
>          BlockDriverState *p_bs = bs->backing->bs;
>  
> +        if (strcmp(p_bs->drv->format_name, "vmdk")) {
> +            /* Backing file is not in vmdk format, so it does not have
> +             * a CID, which makes the overlay's parent CID invalid */
> +            return 0;
> +        }
> +

Reviewed-by: Fam Zheng <famz@redhat.com>

>          if (vmdk_read_cid(p_bs, 0, &cur_pcid) != 0) {
>              /* read failure: report as not valid */
>              return 0;
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH 0/2] vmdk: Fix possible segfault with non-VMDK backing

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On 2018-07-02 23:07, Max Reitz wrote:
> The VMDK driver assumes that its backing file is always a VMDK file,
> too, because it verifies that when creating the overlay.  However, that
> verification means nothing at runtime, and consequently you can assign
> non-VMDK backing files to a VMDK node.  This then breaks because the
> driver accesses the backing node as a VMDK node to read its supposed CID
> to compare it with the overlay's parentCID entry -- which usually fails,
> either in a benign way (we read from a garbage offset, and then we read
> garbage or get a read error straight away), or we get a segfault
> (because the backing node does not have a respective file child).
> 
> Anyway, we just shouldn't do it and instead check whether the backing
> file is a VMDK node before treating it like one.
> 
> 
> (This fixes
>  http://lists.nongnu.org/archive/html/qemu-block/2018-06/msg01268.html)
> 
> 
> Max Reitz (2):
>   vmdk: Fix possible segfault with non-VMDK backing
>   iotests: Add VMDK backing file correlation test
> 
>  block/vmdk.c               |   6 ++
>  tests/qemu-iotests/225     | 132 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/225.out |  24 +++++++
>  tests/qemu-iotests/group   |   1 +
>  4 files changed, 163 insertions(+)
>  create mode 100755 tests/qemu-iotests/225
>  create mode 100644 tests/qemu-iotests/225.out

Applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]