[libvirt] [PATCH 2/2] qemu: Refactor initialisation of security drivers.

[Peter Krempa]

The security driver loading code in qemu has a flaw that causes it to
register the DAC security driver twice. This causes problems (machines
unable to start) as the two DAC drivers clash together.

This patch refactors the code to allow loading the DAC driver even if
its specified in configuration (it can't be registered as a common
security driver), and does not add the driver twice.
---
 src/qemu/qemu_driver.c | 114 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 955744a..9a25253 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -249,40 +249,69 @@ static int
 qemuSecurityInit(struct qemud_driver *driver)
 {
     char **names;
-    char *primary;
-    virSecurityManagerPtr mgr, nested, stack = NULL;
-
-    if (driver->securityDriverNames == NULL)
-        primary = NULL;
-    else
+    char *primary = NULL;
+    virSecurityManagerPtr mgr = NULL;
+    virSecurityManagerPtr nested = NULL;
+    virSecurityManagerPtr stack = NULL;
+    bool hasDAC = false;
+
+    /* set the name of the primary security driver */
+    if (driver->securityDriverNames)
         primary = driver->securityDriverNames[0];

-    /* Create primary driver */
-    mgr = virSecurityManagerNew(primary,
-                                QEMU_DRIVER_NAME,
-                                driver->allowDiskFormatProbing,
-                                driver->securityDefaultConfined,
-                                driver->securityRequireConfined);
+    /* add primary security driver */
+    if ((primary == NULL && driver->privileged) ||
+        STREQ_NULLABLE(primary, "dac")) {
+        if (!driver->privileged) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("DAC security driver usable only when "
+                             "running privileged (as root)"));
+            goto error;
+        }
+
+        mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
+                                       driver->user,
+                                       driver->group,
+                                       driver->allowDiskFormatProbing,
+                                       driver->securityDefaultConfined,
+                                       driver->securityRequireConfined,
+                                       driver->dynamicOwnership);
+        hasDAC = true;
+    } else {
+        mgr = virSecurityManagerNew(primary,
+                                    QEMU_DRIVER_NAME,
+                                    driver->allowDiskFormatProbing,
+                                    driver->securityDefaultConfined,
+                                    driver->securityRequireConfined);
+    }
+
     if (!mgr)
         goto error;

-    /* If a DAC driver is required or additional drivers are provived, a stack
-     * driver should be create to group them all */
-    if (driver->privileged ||
+    /* We need a stack to group the security drivers if:
+     * - additional drivers are provived in configuration
+     * - the primary driver isn't DAC and we are running privileged
+     */
+    if ((driver->privileged && !hasDAC) ||
         (driver->securityDriverNames && driver->securityDriverNames[1])) {
-        stack = virSecurityManagerNewStack(mgr);
-        if (!stack)
+        if (!(stack = virSecurityManagerNewStack(mgr)))
             goto error;
         mgr = stack;
     }

-    /* Loop through additional driver names and add a secudary driver to each
-     * one */
+    /* Loop through additional driver names and add them as nested */
     if (driver->securityDriverNames) {
         names = driver->securityDriverNames + 1;
         while (names && *names) {
             if (STREQ("dac", *names)) {
                 /* A DAC driver has specific parameters */
+                if (!driver->privileged) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                   _("DAC security driver usable only when "
+                                     "running privileged (as root)"));
+                    goto error;
+                }
+
                 nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
                                                   driver->user,
                                                   driver->group,
@@ -290,6 +319,7 @@ qemuSecurityInit(struct qemud_driver *driver)
                                                   driver->securityDefaultConfined,
                                                   driver->securityRequireConfined,
                                                   driver->dynamicOwnership);
+                hasDAC = true;
             } else {
                 nested = virSecurityManagerNew(*names,
                                                QEMU_DRIVER_NAME,
@@ -297,39 +327,32 @@ qemuSecurityInit(struct qemud_driver *driver)
                                                driver->securityDefaultConfined,
                                                driver->securityRequireConfined);
             }
-            if (nested == NULL)
+
+            if (!nested)
                 goto error;
+
             if (virSecurityManagerStackAddNested(stack, nested))
                 goto error;
+
+            nested = NULL;
             names++;
         }
     }

-    if (driver->privileged) {
-        /* When a DAC driver is required, check if there is already one in the
-         * additional drivers */
-        names = driver->securityDriverNames;
-        while (names && *names) {
-            if (STREQ("dac", *names)) {
-               break;
-            }
-            names++;
-        }
-        /* If there is no DAC driver, create a new one and add it to the stack
-         * manager */
-        if (names == NULL || *names == NULL) {
-            nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
-                                              driver->user,
-                                              driver->group,
-                                              driver->allowDiskFormatProbing,
-                                              driver->securityDefaultConfined,
-                                              driver->securityRequireConfined,
-                                              driver->dynamicOwnership);
-            if (nested == NULL)
-                goto error;
-            if (virSecurityManagerStackAddNested(stack, nested))
-                goto error;
-        }
+    if (driver->privileged && !hasDAC) {
+        if (!(nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
+                                                driver->user,
+                                                driver->group,
+                                                driver->allowDiskFormatProbing,
+                                                driver->securityDefaultConfined,
+                                                driver->securityRequireConfined,
+                                                driver->dynamicOwnership)))
+            goto error;
+
+        if (virSecurityManagerStackAddNested(stack, nested))
+            goto error;
+
+        nested = NULL;
     }

     driver->securityManager = mgr;
@@ -338,6 +361,7 @@ qemuSecurityInit(struct qemud_driver *driver)
 error:
     VIR_ERROR(_("Failed to initialize security drivers"));
     virSecurityManagerFree(mgr);
+    virSecurityManagerFree(nested);
     return -1;
 }

-- 
1.7.12