On Wed, Feb 26, 2020 at 20:39:28 -0600, 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). 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.
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.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
qemu-deprecated.texi | 15 +++++++++++++++
block.c | 21 ++++++++++++++++++++-
qemu-img.c | 8 +++++++-
tests/qemu-iotests/114 | 4 ++--
tests/qemu-iotests/114.out | 1 +
5 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 66eca3a1dede..f99b49addccc 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -309,6 +309,21 @@ The above, converted to the current supported format:
@section Related binaries
+@subsection qemu-img backing file without format (since 5.0.0)
+
+The use of @command{qemu-img create}, @command{qemu-img rebase},
+@command{qemu-img convert}, or @command{qemu-img ament} to create or
s/ament/amend/
+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 the guest a potential security exploit if a future probe
+sees non-raw. To avoid warning messages, or even future refusal to
+create an unsafe image, you must pass @option{-o backing_fmt=} (or
+shorthand @option{-F}) to specify the intended backing format. You
+may use @command{qemu-img rebase -u} to retroactively add a backing
+format to an existing image.
I'd add a note for users who wish to fix existing images (and I need
to add it to libvirt's knowledge base too) that when the user is unsure
of the backing image format and desn't trust that the image was handled
in a trusted way, they must not use the format detected by qemu-img info
if the image specifies a backing file, unless they also trust the
backing file. (Sorry as a non-native English speaker I can't express
this in a more elegant way.).
@subsection qemu-img convert -n -o (since 4.2.0)
All options specified in @option{-o} are image creation options, so
[...]
diff --git a/qemu-img.c b/qemu-img.c
index b9375427404d..e75ec1bdb555 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3637,7 +3637,13 @@ static int img_rebase(int argc, char **argv)
* doesn't change when we switch the backing file.
*/
if (out_baseimg && *out_baseimg) {
- ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false);
+ if (blk_new_backing && !out_basefmt) {
+ out_basefmt = blk_bs(blk_new_backing)->drv->format_name;
+ warn_report("Deprecated use of backing file "
+ "without explicit backing format, using "
+ "detected format of %s", out_basefmt);
Isn't this a similar instance to what I've reported in the previous
version? You warn that the format is missing, but set out_basefmt to the
detected format , thus bdrv_change_backing_file will then write the
detected format into the overlay even when it previously did not?
I think this has to have the same check for raw-only as above.
+ }
+ ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true);
or just the above line.
} else {
ret = bdrv_change_backing_file(bs, NULL, NULL, false);
}
I feel that this deprecation will be at least partially controversial,
but in my opinion it's the correct thing to do.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
after you address the issue above.