From 16fb3c8b83fb39f9931e942f7d1738ffe024d234 Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Fri, 30 Aug 2019 14:34:12 +0200
Subject: [PATCH] qemu_blockjob: Remove secdriver metadata more frequently

If a block job reaches failed/cancelled state, or is completed
without pivot then we must remove security driver metadata
associated to the backing chain so that we don't leave any
metadata behind.

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

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_blockjob.c | 59 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 1b22689e0c..a991309ee7 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -659,7 +659,23 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
         disk->src = disk->mirror;
     } else {
         if (disk->mirror) {
+            virStorageSourcePtr n;
+
             virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
+
+            /* Ideally, we would restore seclabels on the backing chain here
+             * but we don't know if somebody else is not using parts of it.
+             * Remove security driver metadata so that they are not leaked. */
+            for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) {
+                if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
+                    VIR_WARN("Unable to remove disk metadata on "
+                             "vm %s from %s (disk target %s)",
+                             vm->def->name,
+                             NULLSTR(disk->src->path),
+                             disk->dst);
+                }
+            }
+
             virObjectUnref(disk->mirror);
         }
     }
@@ -728,7 +744,23 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver,
     case VIR_DOMAIN_BLOCK_JOB_FAILED:
     case VIR_DOMAIN_BLOCK_JOB_CANCELED:
         if (disk->mirror) {
+            virStorageSourcePtr n;
+
             virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
+
+            /* Ideally, we would restore seclabels on the backing chain here
+             * but we don't know if somebody else is not using parts of it.
+             * Remove security driver metadata so that they are not leaked. */
+            for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) {
+                if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
+                    VIR_WARN("Unable to remove disk metadata on "
+                             "vm %s from %s (disk target %s)",
+                             vm->def->name,
+                             NULLSTR(disk->src->path),
+                             disk->dst);
+                }
+            }
+
             virObjectUnref(disk->mirror);
             disk->mirror = NULL;
         }
@@ -1128,16 +1160,33 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver,
 
 
 static void
-qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm,
+qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver,
+                                           virDomainObjPtr vm,
                                            qemuBlockJobDataPtr job)
 {
+    virDomainDiskDefPtr disk = job->disk;
+    virStorageSourcePtr n;
+
     VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name);
 
-    if (!job->disk)
+    if (!disk)
         return;
 
-    virObjectUnref(job->disk->mirror);
-    job->disk->mirror = NULL;
+    /* Ideally, we would make the backing chain read only again (yes, SELinux
+     * can do that using different labels). But that is not implemented yet and
+     * not leaking security driver metadata is more important. */
+    for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) {
+        if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
+            VIR_WARN("Unable to remove disk metadata on "
+                     "vm %s from %s (disk target %s)",
+                     vm->def->name,
+                     NULLSTR(disk->src->path),
+                     disk->dst);
+        }
+    }
+
+    virObjectUnref(disk->mirror);
+    disk->mirror = NULL;
 }
 
 
@@ -1231,7 +1280,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job,
             break;
 
         case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
-            qemuBlockJobProcessEventFailedActiveCommit(vm, job);
+            qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job);
             break;
 
         case QEMU_BLOCKJOB_TYPE_CREATE:
-- 
GitLab