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

Eric Blake eblake at redhat.com
Thu Feb 27 13:13:42 UTC 2020


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 <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/
> 

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 <pkrempa at redhat.com>
> 
> after you address the issue above.
> 

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




More information about the libvir-list mailing list