Re: [PATCH v2 2/3] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 25, 2020 at 02:25:44PM +0100, 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
> are able to probe the image, the probing result was successful and the
> probed image does not have any backing or data file.

Ok, so we're limiting probing to 1 level of recursion, so we're
not newly enabling 3 level chains

> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> Users thus don't have to re-invent when image format detection is safe
> to do.
> 
> 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.
> 
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
>  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 d75d2a689a..b133cf17f1 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -4986,6 +4986,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>                                   virHashTablePtr cycle,
>                                   unsigned int depth)
>  {
> +    virStorageFileFormat orig_format = src->format;
>      size_t headerLen;
>      int rv;
>      g_autofree char *buf = NULL;
> @@ -4995,10 +4996,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)
> @@ -5007,6 +5015,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>      if (virStorageFileGetMetadataInternal(src, buf, headerLen) < 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. */
> +    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;
> +        }
> +    }

Ok, this is where we limit recursion to 1 level to avoid enabling
deep chains.

> +
>      if (src->backingStoreRaw) {
>          if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
>              return -1;
> @@ -5015,33 +5035,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>          if (rv == 1)
>              return 0;
> 
> -        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));
> -                return -1;
>              }
> 
> -            backingStore->format = VIR_STORAGE_FILE_RAW;
> -        }

And this is just about improved error reporting

> -
> -        if (backingStore->format == VIR_STORAGE_FILE_AUTO_SAFE)
> -            backingStore->format = VIR_STORAGE_FILE_AUTO;
> -
> -        if (virStorageFileGetMetadataRecurse(backingStore, parent,
> -                                             uid, gid,
> -                                             report_broken,
> -                                             cycle, depth + 1) < 0) {
> -            if (report_broken)
> -                return -1;
> -            else
> -                return 0;
> +            return -1;
>          }
> 
>          backingStore->id = depth;

Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux