Re: [libvirt] [PATCH] Fix configuration of QEMU security drivers

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; 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: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 135 +++++++++++++--------------------------- src/security/security_manager.c | 8 ++- src/security/security_stack.c | 38 ++++------- src/security/security_stack.h | 8 --- 4 files changed, 61 insertions(+), 128 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a25253..374349a 100644 --- a/src/qemu/qemu_driver.c +++ b/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; } diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 0e106d5..367f7ad 100644 --- a/src/security/security_manager.c +++ b/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; } diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 7dcd626..0eb7e76 100644 --- a/src/security/security_stack.c +++ b/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 diff --git a/src/security/security_stack.h b/src/security/security_stack.h index 6898c03..5bb3be7 100644 --- a/src/security/security_stack.h +++ b/src/security/security_stack.h @@ -27,19 +27,11 @@ 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);