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

[Eric Blake]

On 2/27/20 1:09 AM, Peter Krempa wrote:
> 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 <address@hidden>
> > ---
> >   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/

Fixing.  (Do you ever wonder if I leave a typo in, just to check that 
reviewers were actually reviewing?  But in this case, it was a symptom 
of me posting a series late at night to start the review process, even 
though I _really_ need to post a v3 that adds even more iotest coverage 
of every new deprecation warning made possible in this patch)

> > +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.).

Good idea.  How about:

...to retroactively add a backing format to an existing image.  However, 
be aware that there are already potential security risks to using 
'qemu-img info' to probe the format of an untrusted backing image, when 
deciding what format to write into an image with 'qemu-img rebase -u'.


> >   @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.

Yes, I think I missed a spot (blame the late-night push to get the patch 
out the door).  I'll wait to see if I get review from the qemu side 
before posting v3, though.

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

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org