From 1e3921f1065972dc236bc726f19be37492a145c6 Mon Sep 17 00:00:00 2001

Message-Id: <1e3921f1065972dc236bc726f19be37492a145c6@dist-git>

From: Peter Krempa <pkrempa@redhat.com>

Date: Fri, 28 Feb 2020 10:24:43 +0100

Subject: [PATCH] virStorageFileGetMetadataRecurse: Allow format probing under

 special circumstances

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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.

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

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

(cherry picked from commit ae9e6c2a2b75d958995c661f7bb64ed4353a6404)

https://bugzilla.redhat.com/show_bug.cgi?id=1798148

Message-Id: <77d1d21a0ce64fd49c501b1ed3667307cbab0b73.1582881363.git.pkrempa@redhat.com>

Reviewed-by: Ján Tomko <jtomko@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 0bfd9e16e9..b88763b267 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;

+        }

+    }

+

     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;

-        }

-

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

-- 

2.25.1