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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
after you address the issue above.
