[PATCH v2 3/3] qemu-img: Deprecate use of -b without -F

[Peter Krempa]

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 at 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
> 
> + at subsection qemu-img backing file without format (since 5.0.0)
> +
> +The use of @command{qemu-img create}, @command{qemu-img rebase},
> + at 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 at redhat.com>

after you address the issue above.