[libvirt] [PATCH 0/2] Fix DAC driver domain start regression
The DAC driver was registered twice, breaking starting of machines if no other security driver was used (basicaly on all non-selinux machines). Peter Krempa (2): Revert "security: Add DAC to security_drivers" qemu: Refactor initialisation of security drivers. src/qemu/qemu_driver.c | 114 +++++++++++++++++++++++++---------------- src/security/security_driver.c | 2 - 2 files changed, 69 insertions(+), 47 deletions(-) -- 1.7.12
This reverts commit 9f9b7b85c9b422e8f4e813f3920bf8f433246a4a. The DAC security driver needs special handling and extra parameters and can't just be added to regular security drivers. --- src/security/security_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/security/security_driver.c b/src/security/security_driver.c index e6da220..f450a94 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -35,7 +35,6 @@ # include "security_apparmor.h" #endif -#include "security_dac.h" #include "security_nop.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -47,7 +46,6 @@ static virSecurityDriverPtr security_drivers[] = { #ifdef WITH_SECDRIVER_APPARMOR &virAppArmorSecurityDriver, #endif - &virSecurityDriverDAC, &virSecurityDriverNop, /* Must always be last, since it will always probe */ }; -- 1.7.12
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
On Wed, Aug 29, 2012 at 02:56:13PM +0200, Peter Krempa wrote:
The DAC driver was registered twice, breaking starting of machines if no other security driver was used (basicaly on all non-selinux machines).
Peter Krempa (2): Revert "security: Add DAC to security_drivers" qemu: Refactor initialisation of security drivers.
src/qemu/qemu_driver.c | 114 +++++++++++++++++++++++++---------------- src/security/security_driver.c | 2 - 2 files changed, 69 insertions(+), 47 deletions(-)
Okay, i applied them and it seems to fix the problem ! Code looks okay, so ACK, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/29/12 16:02, Daniel Veillard wrote:
On Wed, Aug 29, 2012 at 02:56:13PM +0200, Peter Krempa wrote:
The DAC driver was registered twice, breaking starting of machines if no other security driver was used (basicaly on all non-selinux machines).
Peter Krempa (2): Revert "security: Add DAC to security_drivers" qemu: Refactor initialisation of security drivers.
src/qemu/qemu_driver.c | 114 +++++++++++++++++++++++++---------------- src/security/security_driver.c | 2 - 2 files changed, 69 insertions(+), 47 deletions(-)
Okay, i applied them and it seems to fix the problem ! Code looks okay, so ACK,
thanks,
Pushed, thanks! Peter
Daniel
participants (2)
-
Daniel Veillard -
Peter Krempa