[PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

Eric Blake eblake at redhat.com
Wed Feb 19 16:21:00 UTC 2020


On 2/17/20 11:13 AM, Peter Krempa wrote:
> Allow format probing to work around lazy clients which did not specify
> their format in the overlay. Format probing will be allowed only, if we

s/only, if/only if/

> are able to probe the image, the probing result was successful and the
> probed image does not have any backing or data file.
> 
> This relaxes the restrictions which were imposed in commit 3615e8b39bad
> in cases when we know that the image probing will not result in security
> issues or data corruption.

It took me a few minutes of thinking about this.

Scenario 1:

base.raw <- wrap.qcow2

where wrap.qcow2 specifies backing of base.raw but not the format.  If 
we probe, we can have a couple of outcomes:

1a: base.raw probes as raw (the probed image has no backing or data 
file), using it as raw is safe (it matches what wrap.qcow2 should have 
specified but didn't, and we aren't changing the data the guest would 
read nor are we opening unexpected files)

1b: base.raw probes as qcow2 (because of whatever the guest wrote 
there), using it as qcow2 is wrong - the guest will see corrupted data. 
What's more, if the probe sees it as qcow2 with backing file, and we 
open the backing file, it also has security implications.

Scenario 2:

base.qcow2 <- wrap.qcow2

where wrap.qcow2 specifies backing of base.qcow2 but not the format.  If 
we probe, we will always have just one outcome:

2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if 
base.qcow2 has a further backing image, the backing chain is now 
dependent on a probe.

Since 1b and 2a have the same probe result, but massively different data 
corruption and/or security concerns, it is NOT sufficient to claim that 
a probe was safe merely because "the probed image does not have any 
backing or data file".  It is ONLY safe if the probe turns up raw.  Any 
other probed format is inherently unsafe.

> 
> We perform the image format detection and in the case that we were able
> to probe the format and the format does not specify a backing store (or
> doesn't support backing store) we can use this format.

Wrong.  The condition needs to be:

If we probe the format, and the probe returns raw, then it is safe to 
use raw as the format.

> 
> With pre-blockdev configurations this will restore the previous
> behaviour for the images mentioned above as qemu would probe the format
> anyways. It also improves error reporting compared to the old state as
> we now report that the backing chain will be broken in case when there
> is a backing file.

Improved error reporting because the probe returned qcow2 that would 
have required us to chase a backing file is fine; but while blindly 
accepting a qcow2 probe result when there is no backing file might avoid 
the security issue of chasing a backing file under guest control, it 
does not solve the data corruption issue of interpreting data as qcow2 
that should have been interpreted as raw.

> 
> In blockdev configurations this ensures that libvirt will not cause data
> corruption by ending the chain prematurely without notifying the user,
> but still allows the old semantics when the users forgot to specify the
> format.

The only time where it is safe to imply a forgotten format is if the 
probed format is still raw.

> 
> The price for this is that libvirt will need to keep the image format
> detector still current and working or replace it by invocation of
> qemu-img.

Maybe.  Any format that qemu recognizes but libvirt does not risks a 
case where libvirt probes the image as raw but lets qemu re-probe the 
image and then qemu exposes different data.  But as long as libvirt 
always passes explicit format to qemu (including explicit raw format of 
a backing file whose format was forgotten but probing said it was raw), 
then it doesn't matter what other formats libvirt can probe for.  The 
only benefit for libvirt probing formats is then for better error 
messages for non-raw.

> 
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
>   src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index b984204b93..bbdf7be094 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -5010,6 +5010,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>                                    virHashTablePtr cycle,
>                                    unsigned int depth)
>   {
> +    virStorageFileFormat orig_format = src->format;
>       size_t headerLen;
>       int backingFormat;
>       int rv;
> @@ -5020,10 +5021,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>                 NULLSTR(src->path), src->format,
>                 (unsigned int)uid, (unsigned int)gid);
> 
> +    if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
> +        src->format = VIR_STORAGE_FILE_AUTO;
> +
>       /* exit if we can't load information about the current image */
>       rv = virStorageFileSupportsBackingChainTraversal(src);
> -    if (rv <= 0)
> +    if (rv <= 0) {
> +        if (orig_format == VIR_STORAGE_FILE_AUTO)
> +            return -2;
> +
>           return rv;
> +    }
> 
>       if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
>                                                      &buf, &headerLen, cycle) < 0)
> @@ -5032,6 +5040,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>       if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) < 0)
>           return -1;
> 
> +    /* If we probed the format we MUST ensure that nothing else than the current
> +     * image (this includes both backing files and external data store) is
> +     * considered for security labelling and/or recursion. */

Grammar:

If we probed the format, we MUST ensure that nothing besides the current 
image (including both backing files and external data store) will be 
considered for security labelling and/or recursion.

> +    if (orig_format == VIR_STORAGE_FILE_AUTO) {
> +        if (src->backingStoreRaw || src->externalDataStoreRaw) {
> +            src->format = VIR_STORAGE_FILE_RAW;
> +            VIR_FREE(src->backingStoreRaw);
> +            VIR_FREE(src->externalDataStoreRaw);
> +            return -2;
> +        }
> +    }
> +
>       if (src->backingStoreRaw) {
>           if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
>               return -1;
> @@ -5042,33 +5062,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
> 
>           backingStore->format = backingFormat;
> 
> -        if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> -            /* Assuming the backing store to be raw can lead to failures. We do
> -             * it only when we must not report an error to prevent losing VMs.
> -             * Otherwise report an error.
> -             */
> -            if (report_broken) {
> +        if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
> +                                                   uid, gid,
> +                                                   report_broken,
> +                                                   cycle, depth + 1)) < 0) {
> +            if (!report_broken)
> +                return 0;
> +
> +            if (rv == -2) {
>                   virReportError(VIR_ERR_OPERATION_INVALID,
>                                  _("format of backing image '%s' of image '%s' was not specified in the image metadata "
>                                    "(See https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
>                                  src->backingStoreRaw, NULLSTR(src->path));

I disagree with the logic here.  What we really need is:

If the backing format was not specified, we probe to see what is there. 
If the result of that probe is raw, it is safe to treat the image as 
raw.  If the result is anything else, we must report an error stating 
that what we probed could not be trusted unless the user adds an 
explicit backing format (either confirming that our probe was correct, 
or with the correct format overriding what we mis-probed).

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