Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F

[Kashyap Chamarthy]

On Fri, Mar 06, 2020 at 04:51:21PM -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, 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).

I happily welcome this change.  I'm going around and fixing various docs
of differnt projects that create overlays without explicitly spelling
out backing files.  (FWIW, I also make sure to mention this, in context,
in all QEMU-related talks I give publicliy.)

This proactive action from QEMU will definitely help.

> 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.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  docs/system/deprecated.rst | 19 +++++++++++++++++++
>  block.c                    | 21 ++++++++++++++++++++-
>  qemu-img.c                 |  2 +-
>  tests/qemu-iotests/114     | 11 +++++++++++
>  tests/qemu-iotests/114.out |  8 ++++++++
>  5 files changed, 59 insertions(+), 2 deletions(-)

Before (with: qemu-4.2.0-2.fc30):

    $> qemu-img create -f qcow2 -b ./base.raw ./overlay1.qcow2
    Formatting './overlay1.qcow2', fmt=qcow2 size=4294967296 
backing_file=./base.raw cluster_size=65536 lazy_refcounts=off refcount_bits=16

After (with the patch series applied to QEMU Git):

    $> git describe
    v4.2.0-2204-gd6c7830114

    # Create; *without* specifying "-F raw"
    $> ~/build/qemu/qemu-img create -f qcow2 -b ./base.raw ./overlay2.qcow2
    qemu-img: warning: Deprecated use of backing file without explicit backing 
format (detected format of raw)
    Formatting './overlay2.qcow2', fmt=qcow2 size=4294967296 
backing_file=./base.raw backing_fmt=raw cluster_size=65536 lazy_refcounts=off 
refcount_bits=16

    # Rebase; *without* specifying "-F raw"
    $> ~/build/qemu/qemu-img rebase -b base.raw overlay1.qcow2
    qemu-img: warning: Deprecated use of backing file without explicit backing 
format, use of this image requires potentially unsafe format probing


However, for the "Convert" case, is it correct that no warning is thrown
for the below?

    $> ~/build/qemu/qemu-img info overlay1.qcow2 
    image: overlay1.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
    
    
    $> ~/build/qemu/qemu-img convert -f qcow2 -O qcow2 overlay1.qcow2 
flattened.raw

    $> echo $?
    0

> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 6c1d9034d9e3..a8ffacf54a52 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -376,6 +376,25 @@ The above, converted to the current supported format::
>  Related binaries
>  ----------------
> 
> +qemu-img backing file without format (since 5.0.0)
> +''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
> +convert``, or ``qemu-img amend`` 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.

Nit: s/qemu/QEMU/g/

Ultra Nit: should this paragraph be broken down into two?  Experience
tells people usually feel deterred read "substantial paragraphs" :-)

I'll report back the Amend case.  (And once I get clarification on the
Convert scenario, I'll be happy to give Tested-by.)

[...]

-- 
/kashyap