[PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files

Alberto Garcia posted 1 patch 4 years ago
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200327164857.31415-1-berto@igalia.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
block/qcow2.c              |  6 +++
tests/qemu-iotests/060     | 12 ++---
tests/qemu-iotests/060.out |  2 -
tests/qemu-iotests/290     | 94 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/290.out | 57 +++++++++++++++++++++++
tests/qemu-iotests/group   |  1 +
6 files changed, 163 insertions(+), 9 deletions(-)
create mode 100755 tests/qemu-iotests/290
create mode 100644 tests/qemu-iotests/290.out
[PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Alberto Garcia 4 years ago
A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
v3:
- Rebase and change iotest number
- Show output of qemu-img map in iotest 290 [Kevin]
- Use the l2_offset and rb_offset variables in iotest 060

v2:

- Don't create the image with compat=0.10 in iotest 060 [Max]
- Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
- Add list of unsupported options to iotest 289 [Max]

 block/qcow2.c              |  6 +++
 tests/qemu-iotests/060     | 12 ++---
 tests/qemu-iotests/060.out |  2 -
 tests/qemu-iotests/290     | 94 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/290.out | 57 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 163 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/290
 create mode 100644 tests/qemu-iotests/290.out

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..e8cbcc1ec1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3784,6 +3784,12 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
     int ret;
     BDRVQcow2State *s = bs->opaque;
 
+    /* If the image does not support QCOW_OFLAG_ZERO then discarding
+     * clusters could expose stale data from the backing file. */
+    if (s->qcow_version < 3 && bs->backing) {
+        return -ENOTSUP;
+    }
+
     if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
         assert(bytes < s->cluster_size);
         /* Ignore partial clusters, except for the special case of the
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 043f12904a..32c0ecce9e 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -160,18 +160,16 @@ TEST_IMG=$BACKING_IMG _make_test_img 1G
 
 $QEMU_IO -c 'write 0k 64k' "$BACKING_IMG" | _filter_qemu_io
 
-# compat=0.10 is required in order to make the following discard actually
-# unallocate the sector rather than make it a zero sector - we want COW, after
-# all.
-_make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
+_make_test_img -b "$BACKING_IMG" 1G
 # Write two clusters, the second one enforces creation of an L2 table after
 # the first data cluster.
 $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
 # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$l2_offset" "\x00\x00\x00\x00\x00\x00\x00\x00"
+poke_file "$TEST_IMG" "$(($rb_offset+10))" "\x00\x00"
 # Now, corrupt the image by marking the second L2 table cluster as free.
-poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
+poke_file "$TEST_IMG" "$(($rb_offset+12))" "\x00\x00"
 # Start a write operation requiring COW on the image stopping it right before
 # doing the read; then, trigger the corruption prevention by writing anything to
 # any unallocated cluster, leading to an attempt to overwrite the second L2
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index d27692a33c..09caaea865 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -105,8 +105,6 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 536870912
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-discard 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with active L2 table); further corruption events will be suppressed
 blkdebug: Suspended request '0'
 write failed: Input/output error
diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
new file mode 100755
index 0000000000..e41d642c7f
--- /dev/null
+++ b/tests/qemu-iotests/290
@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# 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=berto@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1    # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts 'compat=0.10' refcount_bits data_file
+
+echo
+echo "### Test 'qemu-io -c discard' on a QCOW2 image without a backing file"
+echo
+for qcow2_compat in 0.10 1.1; do
+    echo "# Create an image with compat=$qcow2_compat without a backing file"
+    _make_test_img -o "compat=$qcow2_compat" 128k
+
+    echo "# Fill all clusters with data and then discard them"
+    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
+    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
+
+    echo "# Read the data from the discarded clusters"
+    $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
+done
+
+echo
+echo "### Test 'qemu-io -c discard' on a QCOW2 image with a backing file"
+echo
+
+echo "# Create a backing image and fill it with data"
+BACKING_IMG="$TEST_IMG.base"
+TEST_IMG="$BACKING_IMG" _make_test_img 128k
+$QEMU_IO -c 'write -P 0xff 0 128k' "$BACKING_IMG" | _filter_qemu_io
+
+for qcow2_compat in 0.10 1.1; do
+    echo "# Create an image with compat=$qcow2_compat and a backing file"
+    _make_test_img -o "compat=$qcow2_compat" -b "$BACKING_IMG"
+
+    echo "# Fill all clusters with data and then discard them"
+    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
+    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
+
+    echo "# Read the data from the discarded clusters"
+    if [ "$qcow2_compat" = "1.1" ]; then
+        # In qcow2 v3 clusters are zeroed (with QCOW_OFLAG_ZERO)
+        $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
+    else
+        # In qcow2 v2 if there's a backing image we cannot zero the clusters
+        # without exposing the backing file data so discard does nothing
+        $QEMU_IO -c 'read -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
+    fi
+
+    echo "# Output of qemu-img map"
+    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
new file mode 100644
index 0000000000..8d46e8382f
--- /dev/null
+++ b/tests/qemu-iotests/290.out
@@ -0,0 +1,57 @@
+QA output created by 290
+
+### Test 'qemu-io -c discard' on a QCOW2 image without a backing file
+
+# Create an image with compat=0.10 without a backing file
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+# Fill all clusters with data and then discard them
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Read the data from the discarded clusters
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Create an image with compat=1.1 without a backing file
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+# Fill all clusters with data and then discard them
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Read the data from the discarded clusters
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+### Test 'qemu-io -c discard' on a QCOW2 image with a backing file
+
+# Create a backing image and fill it with data
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Create an image with compat=0.10 and a backing file
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 backing_file=TEST_DIR/t.IMGFMT.base
+# Fill all clusters with data and then discard them
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Read the data from the discarded clusters
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Output of qemu-img map
+Offset          Length          Mapped to       File
+0               0x20000         0x50000         TEST_DIR/t.qcow2
+# Create an image with compat=1.1 and a backing file
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 backing_file=TEST_DIR/t.IMGFMT.base
+# Fill all clusters with data and then discard them
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Read the data from the discarded clusters
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Output of qemu-img map
+Offset          Length          Mapped to       File
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 79c6dfc85d..435dccd5af 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -296,3 +296,4 @@
 286 rw quick
 288 quick
 289 rw quick
+290 rw auto quick
-- 
2.20.1


Re: [PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Eric Blake 4 years ago
On 3/27/20 11:48 AM, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---

> +++ b/tests/qemu-iotests/290

> +
> +echo
> +echo "### Test 'qemu-io -c discard' on a QCOW2 image without a backing file"
> +echo
> +for qcow2_compat in 0.10 1.1; do
> +    echo "# Create an image with compat=$qcow2_compat without a backing file"
> +    _make_test_img -o "compat=$qcow2_compat" 128k
> +
> +    echo "# Fill all clusters with data and then discard them"
> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
> +
> +    echo "# Read the data from the discarded clusters"
> +    $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
> +done

Should this loop also inspect qemu-img map output?

> +
> +echo
> +echo "### Test 'qemu-io -c discard' on a QCOW2 image with a backing file"
> +echo
> +
> +echo "# Create a backing image and fill it with data"
> +BACKING_IMG="$TEST_IMG.base"
> +TEST_IMG="$BACKING_IMG" _make_test_img 128k
> +$QEMU_IO -c 'write -P 0xff 0 128k' "$BACKING_IMG" | _filter_qemu_io
> +
> +for qcow2_compat in 0.10 1.1; do
> +    echo "# Create an image with compat=$qcow2_compat and a backing file"
> +    _make_test_img -o "compat=$qcow2_compat" -b "$BACKING_IMG"
> +
> +    echo "# Fill all clusters with data and then discard them"
> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
> +
> +    echo "# Read the data from the discarded clusters"
> +    if [ "$qcow2_compat" = "1.1" ]; then
> +        # In qcow2 v3 clusters are zeroed (with QCOW_OFLAG_ZERO)
> +        $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    else
> +        # In qcow2 v2 if there's a backing image we cannot zero the clusters
> +        # without exposing the backing file data so discard does nothing
> +        $QEMU_IO -c 'read -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
> +    fi
> +
> +    echo "# Output of qemu-img map"
> +    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
> +done

But I agree this was the more interesting one, so we at least have 
decent coverage of the change itself.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Alberto Garcia 4 years ago
On Fri 27 Mar 2020 07:13:04 PM CET, Eric Blake wrote:
>> +for qcow2_compat in 0.10 1.1; do
>> +    echo "# Create an image with compat=$qcow2_compat without a backing file"
>> +    _make_test_img -o "compat=$qcow2_compat" 128k
>> +
>> +    echo "# Fill all clusters with data and then discard them"
>> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
>> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
>> +
>> +    echo "# Read the data from the discarded clusters"
>> +    $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
>> +done
>
> Should this loop also inspect qemu-img map output?

I guess we can, although here the image is completely unallocated in
both cases.

Berto

Re: [PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Eric Blake 4 years ago
On 3/27/20 1:43 PM, Alberto Garcia wrote:
> On Fri 27 Mar 2020 07:13:04 PM CET, Eric Blake wrote:
>>> +for qcow2_compat in 0.10 1.1; do
>>> +    echo "# Create an image with compat=$qcow2_compat without a backing file"
>>> +    _make_test_img -o "compat=$qcow2_compat" 128k
>>> +
>>> +    echo "# Fill all clusters with data and then discard them"
>>> +    $QEMU_IO -c 'write -P 0x01 0 128k' "$TEST_IMG" | _filter_qemu_io
>>> +    $QEMU_IO -c 'discard 0 128k' "$TEST_IMG" | _filter_qemu_io
>>> +
>>> +    echo "# Read the data from the discarded clusters"
>>> +    $QEMU_IO -c 'read -P 0x00 0 128k' "$TEST_IMG" | _filter_qemu_io
>>> +done
>>
>> Should this loop also inspect qemu-img map output?
> 
> I guess we can, although here the image is completely unallocated in
> both cases.

Which shows that even for v2 images, discard DOES do something when 
there is no backing file (even if it is now a no-op when there is a 
backing file after this patch).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Eric Blake 4 years ago
On 3/27/20 11:48 AM, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---

> +++ b/block/qcow2.c
> @@ -3784,6 +3784,12 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
>   
> +    /* If the image does not support QCOW_OFLAG_ZERO then discarding
> +     * clusters could expose stale data from the backing file. */
> +    if (s->qcow_version < 3 && bs->backing) {
> +        return -ENOTSUP;
> +    }
> +

Hmm. Should we blindly always fail for v2, or can we be a little bit 
smarter and still discard a cluster in the top layer if the backing 
layer does not also have it allocated?  In other words, is it also worth 
checking bdrv_is_allocated(), and avoiding the -ENOTSUP failure in the 
case where the backing chain does not have any allocation of the same 
range (at which point, discarding the cluster in the top layer will read 
as zeroes rather than as stale data)?

But that's a minor optimization for an older file format; it's just as 
easy to tell users that if they want maximum discarding power on their 
active layer, then they should be using v3 images.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH v3] qcow2: Forbid discard in qcow2 v2 images with backing files
Posted by Alberto Garcia 4 years ago
On Fri 27 Mar 2020 07:57:40 PM CET, Eric Blake wrote:
>> +    /* If the image does not support QCOW_OFLAG_ZERO then discarding
>> +     * clusters could expose stale data from the backing file. */
>> +    if (s->qcow_version < 3 && bs->backing) {
>> +        return -ENOTSUP;
>> +    }
>
> Hmm. Should we blindly always fail for v2, or can we be a little bit
> smarter and still discard a cluster in the top layer if the backing
> layer does not also have it allocated?

Not sure if that's worth it. I only wanted to fix what looks like a
potential security bug so I prefer to keep it simple. qcow2 v3 has been
out for many years already.

Berto