Fix configuration of QEMU security drivers

If no 'security_driver' config option was set, then the code
just loaded the 'dac' security driver. This is a regression
on previous behaviour, where we would probe for a possible
security driver. ie default to SELinux if available.

This changes things so that it 'security_driver' is not set,
we once again do probing. For simplicity we also always
create the stack driver, even if there is only one driver
active.

The desired semantics are:

 - security_driver not set
     -> probe for selinux/apparmour/nop
     -> auto-add DAC driver
 - security_driver set to a string
     -> add that one driver
     -> auto-add DAC driver
 - security_driver set to a list
     -> add all drivers in list
     -> auto-add DAC driver

It is not allowed, or possible to specify 'dac' in the
security_driver config param, since that is always
enabled.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

src/qemu/qemu_driver.c

@@ -249,119 +249,70 @@ static int
 qemuSecurityInit(struct qemud_driver *driver)
 {
     char **names;
-    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];
-
-    /* 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;
-
-    /* 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])) {
-        if (!(stack = virSecurityManagerNewStack(mgr)))
-            goto error;
-        mgr = stack;
-    }
-
-    /* Loop through additional driver names and add them as nested */
     if (driver->securityDriverNames) {
-        names = driver->securityDriverNames + 1;
+        names = driver->securityDriverNames;
         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,
-                                                  driver->allowDiskFormatProbing,
-                                                  driver->securityDefaultConfined,
-                                                  driver->securityRequireConfined,
-                                                  driver->dynamicOwnership);
+            if (STREQ("dac", *names))
                 hasDAC = true;
-            } else {
-                nested = virSecurityManagerNew(*names,
-                                               QEMU_DRIVER_NAME,
-                                               driver->allowDiskFormatProbing,
-                                               driver->securityDefaultConfined,
-                                               driver->securityRequireConfined);
-            }
-
-            if (!nested)
-                goto error;
 
-            if (virSecurityManagerStackAddNested(stack, nested))
+            if (!(mgr = virSecurityManagerNew(*names,
+                                              QEMU_DRIVER_NAME,
+                                              driver->allowDiskFormatProbing,
+                                              driver->securityDefaultConfined,
+                                              driver->securityRequireConfined)))
                 goto error;
-
-            nested = NULL;
+            if (!stack) {
+                if (!(stack = virSecurityManagerNewStack(mgr)))
+                    goto error;
+            } else {
+                if (virSecurityManagerStackAddNested(stack, mgr) < 0)
+                    goto error;
+            }
+            mgr = NULL;
             names++;
         }
-    }
-
-    if (driver->privileged && !hasDAC) {
-        if (!(nested = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
-                                                driver->user,
-                                                driver->group,
-                                                driver->allowDiskFormatProbing,
-                                                driver->securityDefaultConfined,
-                                                driver->securityRequireConfined,
-                                                driver->dynamicOwnership)))
+    } else {
+        if (!(mgr = virSecurityManagerNew(NULL,
+                                          QEMU_DRIVER_NAME,
+                                          driver->allowDiskFormatProbing,
+                                          driver->securityDefaultConfined,
+                                          driver->securityRequireConfined)))
             goto error;
-
-        if (virSecurityManagerStackAddNested(stack, nested))
+        if (!(stack = virSecurityManagerNewStack(mgr)))
             goto error;
+        mgr = NULL;
+    }
 
-        nested = NULL;
+    if (!hasDAC && driver->privileged) {
+        if (!(mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
+                                             driver->user,
+                                             driver->group,
+                                             driver->allowDiskFormatProbing,
+                                             driver->securityDefaultConfined,
+                                             driver->securityRequireConfined,
+                                             driver->dynamicOwnership)))
+            goto error;
+        if (!stack) {
+            if (!(stack = virSecurityManagerNewStack(mgr)))
+                goto error;
+        } else {
+            if (virSecurityManagerStackAddNested(stack, mgr) < 0)
+                goto error;
+        }
+        mgr = NULL;
     }
 
-    driver->securityManager = mgr;
+    driver->securityManager = stack;
     return 0;
 
 error:
     VIR_ERROR(_("Failed to initialize security drivers"));
+    virSecurityManagerFree(stack);
     virSecurityManagerFree(mgr);
-    virSecurityManagerFree(nested);
     return -1;
 }
 

src/security/security_manager.c

@@ -49,6 +49,12 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr
 {
     virSecurityManagerPtr mgr;
 
+    VIR_DEBUG("drv=%p (%s) virtDriver=%s allowDiskFormatProbing=%d "
+              "defaultConfined=%d requireConfined=%d",
+              drv, drv->name, virtDriver,
+              allowDiskFormatProbing, defaultConfined,
+              requireConfined);
+
     if (VIR_ALLOC_VAR(mgr, char, drv->privateDataLen) < 0) {
         virReportOOMError();
         return NULL;
@@ -80,7 +86,7 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary)
     if (!mgr)
         return NULL;
 
-    virSecurityStackAddPrimary(mgr, primary);
+    virSecurityStackAddNested(mgr, primary);
 
     return mgr;
 }

src/security/security_stack.c

@@ -38,35 +38,31 @@ struct _virSecurityStackItem {
 };
 
 struct _virSecurityStackData {
-    virSecurityManagerPtr primary;
     virSecurityStackItemPtr itemsHead;
 };
 
-int
-virSecurityStackAddPrimary(virSecurityManagerPtr mgr,
-                           virSecurityManagerPtr primary)
-{
-    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
-    if (virSecurityStackAddNested(mgr, primary) < 0)
-        return -1;
-    priv->primary = primary;
-    return 0;
-}
-
 int
 virSecurityStackAddNested(virSecurityManagerPtr mgr,
                           virSecurityManagerPtr nested)
 {
     virSecurityStackItemPtr item = NULL;
     virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItemPtr tmp;
+
+    tmp = priv->itemsHead;
+    while (tmp && tmp->next)
+        tmp = tmp->next;
 
     if (VIR_ALLOC(item) < 0) {
         virReportOOMError();
         return -1;
     }
     item->securityManager = nested;
-    item->next = priv->itemsHead;
-    priv->itemsHead = item;
+    if (tmp)
+        tmp->next = item;
+    else
+        priv->itemsHead = item;
+
     return 0;
 }
 
@@ -74,19 +70,7 @@ virSecurityManagerPtr
 virSecurityStackGetPrimary(virSecurityManagerPtr mgr)
 {
     virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
-    return (priv->primary) ? priv->primary : priv->itemsHead->securityManager;
-}
-
-void virSecurityStackSetPrimary(virSecurityManagerPtr mgr,
-                                virSecurityManagerPtr primary)
-{
-    virSecurityStackAddPrimary(mgr, primary);
-}
-
-void virSecurityStackSetSecondary(virSecurityManagerPtr mgr,
-                                  virSecurityManagerPtr secondary)
-{
-    virSecurityStackAddNested(mgr, secondary);
+    return priv->itemsHead->securityManager;
 }
 
 static virSecurityDriverStatus

src/security/security_stack.h

@@ -26,20 +26,12 @@
 extern virSecurityDriver virSecurityDriverStack;
 
 
-int
-virSecurityStackAddPrimary(virSecurityManagerPtr mgr,
-                           virSecurityManagerPtr primary);
 int
 virSecurityStackAddNested(virSecurityManagerPtr mgr,
                           virSecurityManagerPtr nested);
 virSecurityManagerPtr
 virSecurityStackGetPrimary(virSecurityManagerPtr mgr);
 
-void virSecurityStackSetPrimary(virSecurityManagerPtr mgr,
-                                virSecurityManagerPtr primary);
-void virSecurityStackSetSecondary(virSecurityManagerPtr mgr,
-                                  virSecurityManagerPtr secondary);
-
 virSecurityManagerPtr*
 virSecurityStackGetNested(virSecurityManagerPtr mgr);