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

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&gt; > --- > 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&gt; after you address the issue above.