Re: [PATCH v4 7/7] qemu-img: Deprecate use of -b without -F
On Thu, Mar 12, 2020 at 02:28:22PM -0500, Eric Blake wrote: > Creating an image that requires format probing of the backing image is > inherently unsafe (we've had several CVEs over the years based on > probes leaking information to the guest on a subsequent boot, although > these days tools like libvirt are aware of the issue enough to prevent > the worst effects). However, if our probing algorithm ever changes, > or if other tools like libvirt determine a different probe result than > we do, then subsequent use of that backing file under a different > format will present corrupted data to the guest. Start a deprecation > clock so that future qemu-img can refuse to create unsafe backing > chains that would rely on probing. The warnings are intentionally > emitted from the block layer rather than qemu-img (thus, all paths > into image creation or rewriting perform the check). > > However, there is one time where probing is safe: if we probe raw, > then it is safe to record that implicitly in the image (but we still > warn, as it's better to teach the user to supply -F always than to > make them guess when it is safe). > > iotest 114 specifically wants to create an unsafe image for later > amendment rather than defaulting to our new default of recording a > probed format, so it needs an update. While touching it, expand it to > cover all of the various warnings enabled by this patch. iotest 290 > also shows a change to qcow messages; note that the fact that we now > make a probed format of 'raw' explicit now results in a double > warning, but no one should be creating new qcow images so it is not > worth cleaning up. > > Signed-off-by: Eric Blake > --- > docs/system/deprecated.rst | 19 +++ > block.c| 21 - > qemu-img.c | 2 +- > tests/qemu-iotests/114 | 11 +++ > tests/qemu-iotests/114.out | 8 > tests/qemu-iotests/290.out | 5 - > 6 files changed, 63 insertions(+), 3 deletions(-) [A quick question ... while I'm still testing] I just applied your v4, and I'm here: $> git describe v4.2.0-2399-g3cba0d19f2 Expected warning on 'create' wiht no -F: $> ~/build/v4_tightened_qemu-img-QEMU/qemu-img create -f qcow2 -b ./base.raw ./overlay1.qcow2 qemu-img: warning: Deprecated use of backing file without explicit backing format (detected format of raw) Formatting './overlay1.qcow2', fmt=qcow2 size=4294967296 backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16 But here is the lack of warning with 'convert' expected? $> ~/build/v4_tightened_qemu-img-QEMU/qemu-img convert -B ./base.raw -O qcow2 overlay1.qcow2 flattened.qcow2 In your response on the v3, you said the above should throw a warning; refer to Message-ID: <2fd580c2-4b94-4430-1072-ef04bbd2d...@redhat.com> For completeness' sake: $> ~/build/v4_tightened_qemu-img-QEMU/qemu-img info --backing-chain flattened.qcow2 image: flattened.qcow2 file format: qcow2 virtual size: 4 GiB (4294967296 bytes) disk size: 196 KiB cluster_size: 65536 backing file: ./base.raw Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false image: ./base.raw file format: raw virtual size: 4 GiB (4294967296 bytes) disk size: 778 MiB [...] -- /kashyap
Re: [PATCH v4 7/7] qemu-img: Deprecate use of -b without -F
On 3/12/20 2:28 PM, Eric Blake wrote: Creating an image that requires format probing of the backing image is inherently unsafe (we've had several CVEs over the years based on probes leaking information to the guest on a subsequent boot, although these days tools like libvirt are aware of the issue enough to prevent the worst effects). However, if our probing algorithm ever changes, or if other tools like libvirt determine a different probe result than we do, then subsequent use of that backing file under a different format will present corrupted data to the guest. Start a deprecation clock so that future qemu-img can refuse to create unsafe backing chains that would rely on probing. The warnings are intentionally emitted from the block layer rather than qemu-img (thus, all paths into image creation or rewriting perform the check). However, there is one time where probing is safe: if we probe raw, then it is safe to record that implicitly in the image (but we still warn, as it's better to teach the user to supply -F always than to make them guess when it is safe). iotest 114 specifically wants to create an unsafe image for later amendment rather than defaulting to our new default of recording a probed format, so it needs an update. While touching it, expand it to cover all of the various warnings enabled by this patch. iotest 290 also shows a change to qcow messages; note that the fact that we now make a probed format of 'raw' explicit now results in a double warning, but no one should be creating new qcow images so it is not worth cleaning up. Signed-off-by: Eric Blake --- docs/system/deprecated.rst | 19 +++ Squash this in per Kashyap's v3 review comments: diff --git i/docs/system/deprecated.rst w/docs/system/deprecated.rst index b541d52c7dc0..54a50c45e190 100644 --- i/docs/system/deprecated.rst +++ w/docs/system/deprecated.rst @@ -388,18 +388,19 @@ qemu-img backing file without format (since 5.0.0) The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img convert`` to create or modify an image that depends on a backing file now recommends that an explicit backing format be provided. This is -for safety: if qemu probes a different format than what you thought, +for safety: if QEMU probes a different format than what you thought, the data presented to the guest will be corrupt; similarly, presenting a raw image to a guest allows a potential security exploit if a future -probe sees a non-raw image based on guest writes. To avoid the -warning message, or even future refusal to create an unsafe image, you -must pass ``-o backing_fmt=`` (or the shorthand ``-F`` during create) -to specify the intended backing format. You may use ``qemu-img rebase --u`` to retroactively add a backing format to an existing image. -However, be aware that there are already potential security risks to -blindly using ``qemu-img info`` to probe the format of an untrusted -backing image, when deciding what format to add into an existing -image. +probe sees a non-raw image based on guest writes. + +To avoid the warning message, or even future refusal to create an +unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand +``-F`` during create) to specify the intended backing format. You may +use ``qemu-img rebase -u`` to retroactively add a backing format to an +existing image. However, be aware that there are already potential +security risks to blindly using ``qemu-img info`` to probe the format +of an untrusted backing image, when deciding what format to add into +an existing image. ``qemu-img convert -n -o`` (since 4.2.0) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH v4 7/7] qemu-img: Deprecate use of -b without -F
Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects). However, if our probing algorithm ever changes,
or if other tools like libvirt determine a different probe result than
we do, then subsequent use of that backing file under a different
format will present corrupted data to the guest. Start a deprecation
clock so that future qemu-img can refuse to create unsafe backing
chains that would rely on probing. The warnings are intentionally
emitted from the block layer rather than qemu-img (thus, all paths
into image creation or rewriting perform the check).
However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).
iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update. While touching it, expand it to
cover all of the various warnings enabled by this patch. iotest 290
also shows a change to qcow messages; note that the fact that we now
make a probed format of 'raw' explicit now results in a double
warning, but no one should be creating new qcow images so it is not
worth cleaning up.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 19 +++
block.c| 21 -
qemu-img.c | 2 +-
tests/qemu-iotests/114 | 11 +++
tests/qemu-iotests/114.out | 8
tests/qemu-iotests/290.out | 5 -
6 files changed, 63 insertions(+), 3 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 4261d5589e6a..b541d52c7dc0 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -382,6 +382,25 @@ image). Rather, any changes to the backing chain should
be performed
with ``qemu-img rebase -u`` either before or after the remaining
changes being performed by amend, as appropriate.
+qemu-img backing file without format (since 5.0.0)
+''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
+convert`` to create or modify an image that depends on a backing file
+now recommends that an explicit backing format be provided. This is
+for safety: if qemu probes a different format than what you thought,
+the data presented to the guest will be corrupt; similarly, presenting
+a raw image to a guest allows a potential security exploit if a future
+probe sees a non-raw image based on guest writes. To avoid the
+warning message, or even future refusal to create an unsafe image, you
+must pass ``-o backing_fmt=`` (or the shorthand ``-F`` during create)
+to specify the intended backing format. You may use ``qemu-img rebase
+-u`` to retroactively add a backing format to an existing image.
+However, be aware that there are already potential security risks to
+blindly using ``qemu-img info`` to probe the format of an untrusted
+backing image, when deciding what format to add into an existing
+image.
+
``qemu-img convert -n -o`` (since 4.2.0)
diff --git a/block.c b/block.c
index cd3277ab86fd..69955f53f35a 100644
--- a/block.c
+++ b/block.c
@@ -6064,6 +6064,20 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"Could not open backing image to determine
size.\n");
goto out;
} else {
+if (!backing_fmt) {
+warn_report("Deprecated use of backing file without explicit "
+"backing format (detected format of %s)",
+bs->drv->format_name);
+if (bs->drv == _raw) {
+/*
+ * A probe of raw is always correct, so in this one
+ * case, we can write that into the image.
+ */
+backing_fmt = bs->drv->format_name;
+qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt,
+ NULL);
+}
+}
if (size == -1) {
/* Opened BS, have no size */
size = bdrv_getlength(bs);
@@ -6077,7 +6091,12 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
bdrv_unref(bs);
}
-} /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+/* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+} else if (backing_file && !backing_fmt) {
+warn_report("Deprecated use of unopened backing file without "
+"explicit backing format, use of this
