[libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/8b1c82e32b677246e330a5918c42a6eb4c361ab8.1487082308.git.mprivozn@redhat.com
cfg.mk                    |  5 ++++
src/qemu/qemu_command.c   |  7 +++---
src/qemu/qemu_conf.c      |  9 ++++---
src/qemu/qemu_domain.c    | 17 ++++++-------
src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
src/qemu/qemu_hotplug.c   |  4 +--
src/qemu/qemu_migration.c | 13 +++++-----
src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
9 files changed, 122 insertions(+), 89 deletions(-)
[libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Michal Privoznik 7 years, 1 month ago
Now that we have some qemuSecurity wrappers over
virSecurityManager APIs, lets make sure everybody sticks with
them. We have them for a reason and calling virSecurityManager
API directly instead of wrapper may lead into accidentally
labelling a file on the host instead of namespace.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

This is an alternative approach to:

https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html

 cfg.mk                    |  5 ++++
 src/qemu/qemu_command.c   |  7 +++---
 src/qemu/qemu_conf.c      |  9 ++++---
 src/qemu/qemu_domain.c    | 17 ++++++-------
 src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
 src/qemu/qemu_hotplug.c   |  4 +--
 src/qemu/qemu_migration.c | 13 +++++-----
 src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
 src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
 9 files changed, 122 insertions(+), 89 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 69e3f3a1a..489fda8ea 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -983,6 +983,11 @@ sc_prohibit_sysconf_pagesize:
 	halt='use virGetSystemPageSize[KB] instead of sysconf(_SC_PAGESIZE)' \
 	  $(_sc_search_regexp)
 
+sc_prohibit_virSecurity:
+	@grep -P 'virSecurityManager(?!Ptr)' $$($(VC_LIST_EXCEPT) | grep '^src/qemu/' | \
+		grep -v '^src/qemu/qemu_security') && \
+		{ echo '$(ME): prefer qemuSecurity wrappers' 1>&2; exit 1; } || :
+
 sc_prohibit_pthread_create:
 	@prohibit='\bpthread_create\b' \
 	exclude='sc_prohibit_pthread_create' \
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c00a47a91..110540ba7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -28,6 +28,7 @@
 #include "qemu_capabilities.h"
 #include "qemu_interface.h"
 #include "qemu_alias.h"
+#include "qemu_security.h"
 #include "cpu/cpu.h"
 #include "dirname.h"
 #include "viralloc.h"
@@ -8321,8 +8322,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
     }
 
     for (i = 0; i < tapfdSize; i++) {
-        if (virSecurityManagerSetTapFDLabel(driver->securityManager,
-                                            def, tapfd[i]) < 0)
+        if (qemuSecuritySetTapFDLabel(driver->securityManager,
+                                      def, tapfd[i]) < 0)
             goto cleanup;
         virCommandPassFD(cmd, tapfd[i],
                          VIR_COMMAND_PASS_FD_CLOSE_PARENT);
@@ -8403,7 +8404,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
 
 
 /* NOTE: Not using const virDomainDef here since eventually a call is made
- *       into virSecurityManagerSetTapFDLabel which calls it's driver
+ *       into qemuSecuritySetTapFDLabel which calls it's driver
  *       API domainSetSecurityTapFDLabel that doesn't use the const format.
  */
 static int
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0223a95d2..4fc0dee39 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -38,6 +38,7 @@
 #include "qemu_conf.h"
 #include "qemu_capabilities.h"
 #include "qemu_domain.h"
+#include "qemu_security.h"
 #include "viruuid.h"
 #include "virbuffer.h"
 #include "virconf.h"
@@ -904,7 +905,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
     }
 
     /* access sec drivers and create a sec model for each one */
-    if (!(sec_managers = virSecurityManagerGetNested(driver->securityManager)))
+    if (!(sec_managers = qemuSecurityGetNested(driver->securityManager)))
         goto error;
 
     /* calculate length */
@@ -917,14 +918,14 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
 
     for (i = 0; sec_managers[i]; i++) {
         virCapsHostSecModelPtr sm = &caps->host.secModels[i];
-        doi = virSecurityManagerGetDOI(sec_managers[i]);
-        model = virSecurityManagerGetModel(sec_managers[i]);
+        doi = qemuSecurityGetDOI(sec_managers[i]);
+        model = qemuSecurityGetModel(sec_managers[i]);
         if (VIR_STRDUP(sm->model, model) < 0 ||
             VIR_STRDUP(sm->doi, doi) < 0)
             goto error;
 
         for (j = 0; j < ARRAY_CARDINALITY(virtTypes); j++) {
-            lbl = virSecurityManagerGetBaseLabel(sec_managers[i], virtTypes[j]);
+            lbl = qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]);
             type = virDomainVirtTypeToString(virtTypes[j]);
             if (lbl &&
                 virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f62bf8f1d..2c827ea2c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -588,8 +588,8 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
-                                             vm->def, path) < 0)
+    if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+                                       vm->def, path) < 0)
         goto cleanup;
 
     ret = 0;
@@ -2688,7 +2688,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
     if (qemuDomainRecheckInternalPaths(def, cfg, parseFlags) < 0)
         goto cleanup;
 
-    if (virSecurityManagerVerify(driver->securityManager, def) < 0)
+    if (qemuSecurityVerify(driver->securityManager, def) < 0)
         goto cleanup;
 
     if (qemuDomainDefVcpusPostParse(def) < 0)
@@ -7257,8 +7257,7 @@ qemuDomainSetupDev(virQEMUDriverPtr driver,
 
     VIR_DEBUG("Setting up /dev/ for domain %s", vm->def->name);
 
-    mount_options = virSecurityManagerGetMountOptions(driver->securityManager,
-                                                      vm->def);
+    mount_options = qemuSecurityGetMountOptions(driver->securityManager, vm->def);
 
     if (!mount_options &&
         VIR_STRDUP(mount_options, "") < 0)
@@ -7679,7 +7678,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
     bool delDevice = false;
     bool isLink = S_ISLNK(data->sb.st_mode);
 
-    virSecurityManagerPostFork(data->driver->securityManager);
+    qemuSecurityPostFork(data->driver->securityManager);
 
     if (virFileMakeParentPath(data->file) < 0) {
         virReportSystemError(errno,
@@ -7841,16 +7840,16 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
 #endif
 
     if (STRPREFIX(file, DEVPREFIX)) {
-        if (virSecurityManagerPreFork(driver->securityManager) < 0)
+        if (qemuSecurityPreFork(driver->securityManager) < 0)
             goto cleanup;
 
         if (virProcessRunInMountNamespace(vm->pid,
                                           qemuDomainAttachDeviceMknodHelper,
                                           &data) < 0) {
-            virSecurityManagerPostFork(driver->securityManager);
+            qemuSecurityPostFork(driver->securityManager);
             goto cleanup;
         }
-        virSecurityManagerPostFork(driver->securityManager);
+        qemuSecurityPostFork(driver->securityManager);
     }
 
     if (isLink &&
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89bc833de..096fe36fe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -405,26 +405,26 @@ qemuSecurityInit(virQEMUDriverPtr driver)
         cfg->securityDriverNames[0]) {
         names = cfg->securityDriverNames;
         while (names && *names) {
-            if (!(mgr = virSecurityManagerNew(*names,
-                                              QEMU_DRIVER_NAME,
-                                              flags)))
+            if (!(mgr = qemuSecurityNew(*names,
+                                        QEMU_DRIVER_NAME,
+                                        flags)))
                 goto error;
             if (!stack) {
-                if (!(stack = virSecurityManagerNewStack(mgr)))
+                if (!(stack = qemuSecurityNewStack(mgr)))
                     goto error;
             } else {
-                if (virSecurityManagerStackAddNested(stack, mgr) < 0)
+                if (qemuSecurityStackAddNested(stack, mgr) < 0)
                     goto error;
             }
             mgr = NULL;
             names++;
         }
     } else {
-        if (!(mgr = virSecurityManagerNew(NULL,
-                                          QEMU_DRIVER_NAME,
-                                          flags)))
+        if (!(mgr = qemuSecurityNew(NULL,
+                                    QEMU_DRIVER_NAME,
+                                    flags)))
             goto error;
-        if (!(stack = virSecurityManagerNewStack(mgr)))
+        if (!(stack = qemuSecurityNewStack(mgr)))
             goto error;
         mgr = NULL;
     }
@@ -432,17 +432,17 @@ qemuSecurityInit(virQEMUDriverPtr driver)
     if (virQEMUDriverIsPrivileged(driver)) {
         if (cfg->dynamicOwnership)
             flags |= VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP;
-        if (!(mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME,
-                                             cfg->user,
-                                             cfg->group,
-                                             flags,
-                                             qemuSecurityChownCallback)))
+        if (!(mgr = qemuSecurityNewDAC(QEMU_DRIVER_NAME,
+                                       cfg->user,
+                                       cfg->group,
+                                       flags,
+                                       qemuSecurityChownCallback)))
             goto error;
         if (!stack) {
-            if (!(stack = virSecurityManagerNewStack(mgr)))
+            if (!(stack = qemuSecurityNewStack(mgr)))
                 goto error;
         } else {
-            if (virSecurityManagerStackAddNested(stack, mgr) < 0)
+            if (qemuSecurityStackAddNested(stack, mgr) < 0)
                 goto error;
         }
         mgr = NULL;
@@ -3088,7 +3088,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
     if (fd < 0)
         goto cleanup;
 
-    if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
+    if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
         goto cleanup;
 
     if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
@@ -3553,8 +3553,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr vm,
         return -1;
     }
 
-    if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
-                                          fd) < 0)
+    if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
         return -1;
 
     VIR_FREE(priv->job.current);
@@ -3846,7 +3845,7 @@ qemuDomainScreenshot(virDomainPtr dom,
     }
     unlink_tmp = true;
 
-    virSecurityManagerSetSavedStateLabel(driver->securityManager, vm->def, tmp);
+    qemuSecuritySetSavedStateLabel(driver->securityManager, vm->def, tmp);
 
     qemuDomainObjEnterMonitor(driver, vm);
     if (qemuMonitorScreendump(priv->mon, tmp) < 0) {
@@ -5928,8 +5927,8 @@ static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr secl
      *   QEMU monitor hasn't seen SIGHUP/ERR on poll().
      */
     if (virDomainObjIsActive(vm)) {
-        if (virSecurityManagerGetProcessLabel(driver->securityManager,
-                                              vm->def, vm->pid, seclabel) < 0) {
+        if (qemuSecurityGetProcessLabel(driver->securityManager,
+                                        vm->def, vm->pid, seclabel) < 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            "%s", _("Failed to get security label"));
             goto cleanup;
@@ -5973,8 +5972,7 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
         ret = 0;
     } else {
         int len = 0;
-        virSecurityManagerPtr* mgrs = virSecurityManagerGetNested(
-                                            driver->securityManager);
+        virSecurityManagerPtr* mgrs = qemuSecurityGetNested(driver->securityManager);
         if (!mgrs)
             goto cleanup;
 
@@ -5990,8 +5988,8 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
 
         /* Fill the array */
         for (i = 0; i < len; i++) {
-            if (virSecurityManagerGetProcessLabel(mgrs[i], vm->def, vm->pid,
-                                                  &(*seclabels)[i]) < 0) {
+            if (qemuSecurityGetProcessLabel(mgrs[i], vm->def, vm->pid,
+                                            &(*seclabels)[i]) < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                "%s", _("Failed to get security label"));
                 VIR_FREE(mgrs);
@@ -6369,8 +6367,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
  cleanup:
     virCommandFree(cmd);
     VIR_FREE(errbuf);
-    if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
-                                                 vm->def, path) < 0)
+    if (qemuSecurityRestoreSavedStateLabel(driver->securityManager,
+                                           vm->def, path) < 0)
         VIR_WARN("failed to restore save state label on %s", path);
     virObjectUnref(cfg);
     return ret;
@@ -11196,7 +11194,7 @@ qemuDomainMemoryPeek(virDomainPtr dom,
         goto endjob;
     }
 
-    virSecurityManagerSetSavedStateLabel(driver->securityManager, vm->def, tmp);
+    qemuSecuritySetSavedStateLabel(driver->securityManager, vm->def, tmp);
 
     priv = vm->privateData;
     qemuDomainObjEnterMonitor(driver, vm);
@@ -17064,8 +17062,7 @@ qemuDomainOpenGraphics(virDomainPtr dom,
         goto endjob;
     }
 
-    if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
-                                          fd) < 0)
+    if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
         goto endjob;
 
     qemuDomainObjEnterMonitor(driver, vm);
@@ -17129,13 +17126,13 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (virSecurityManagerSetSocketLabel(driver->securityManager, vm->def) < 0)
+    if (qemuSecuritySetSocketLabel(driver->securityManager, vm->def) < 0)
         goto cleanup;
 
     if (socketpair(PF_UNIX, SOCK_STREAM, 0, pair) < 0)
         goto cleanup;
 
-    if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 0)
+    if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0)
         goto cleanup;
 
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2f209f12b..b99b0e9fb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1134,8 +1134,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
     }
 
     for (i = 0; i < tapfdSize; i++) {
-        if (virSecurityManagerSetTapFDLabel(driver->securityManager,
-                                            vm->def, tapfd[i]) < 0)
+        if (qemuSecuritySetTapFDLabel(driver->securityManager,
+                                      vm->def, tapfd[i]) < 0)
             goto cleanup;
     }
 
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0f4a6cf21..c40cb1391 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -40,6 +40,7 @@
 #include "qemu_cgroup.h"
 #include "qemu_hotplug.h"
 #include "qemu_blockjob.h"
+#include "qemu_security.h"
 
 #include "domain_audit.h"
 #include "virlog.h"
@@ -4597,7 +4598,7 @@ qemuMigrationConnect(virQEMUDriverPtr driver,
     spec->destType = MIGRATION_DEST_FD;
     spec->dest.fd.qemu = -1;
 
-    if (virSecurityManagerSetSocketLabel(driver->securityManager, vm->def) < 0)
+    if (qemuSecuritySetSocketLabel(driver->securityManager, vm->def) < 0)
         goto cleanup;
     if (virNetSocketNewConnectTCP(host, port,
                                   AF_UNSPEC,
@@ -4605,7 +4606,7 @@ qemuMigrationConnect(virQEMUDriverPtr driver,
         spec->dest.fd.qemu = virNetSocketDupFD(sock, true);
         virObjectUnref(sock);
     }
-    if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 0 ||
+    if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0 ||
         spec->dest.fd.qemu == -1)
         goto cleanup;
 
@@ -5076,8 +5077,8 @@ static int doTunnelMigrate(virQEMUDriverPtr driver,
         spec.dest.fd.local = fds[0];
     }
     if (spec.dest.fd.qemu == -1 ||
-        virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
-                                          spec.dest.fd.qemu) < 0) {
+        qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
+                                    spec.dest.fd.qemu) < 0) {
         virReportSystemError(errno, "%s",
                              _("cannot create pipe for tunnelled migration"));
         goto cleanup;
@@ -6463,8 +6464,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
      * doesn't have to open() the file, so while we still have to
      * grant SELinux access, we can do it on fd and avoid cleanup
      * later, as well as skip futzing with cgroup.  */
-    if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
-                                          compressor ? pipeFD[1] : fd) < 0)
+    if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
+                                    compressor ? pipeFD[1] : fd) < 0)
         goto cleanup;
 
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 92fa69b3c..5c44e565b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -221,8 +221,7 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
         return 0;
     }
 
-    if (virSecurityManagerSetDaemonSocketLabel(driver->securityManager,
-                                               vm->def) < 0) {
+    if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0) {
         VIR_ERROR(_("Failed to set security context for agent for %s"),
                   vm->def->name);
         goto cleanup;
@@ -250,8 +249,7 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
         return -1;
     }
 
-    if (virSecurityManagerClearSocketLabel(driver->securityManager,
-                                           vm->def) < 0) {
+    if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0) {
         VIR_ERROR(_("Failed to clear security context for agent for %s"),
                   vm->def->name);
         qemuAgentClose(agent);
@@ -1657,8 +1655,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
     int ret = -1;
     qemuMonitorPtr mon = NULL;
 
-    if (virSecurityManagerSetDaemonSocketLabel(driver->securityManager,
-                                               vm->def) < 0) {
+    if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0) {
         VIR_ERROR(_("Failed to set security context for monitor for %s"),
                   vm->def->name);
         return -1;
@@ -1695,7 +1692,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
     }
     priv->mon = mon;
 
-    if (virSecurityManagerClearSocketLabel(driver->securityManager, vm->def) < 0) {
+    if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0) {
         VIR_ERROR(_("Failed to clear security context for monitor for %s"),
                   vm->def->name);
         return -1;
@@ -2638,7 +2635,7 @@ static int qemuProcessHook(void *data)
      * protected across fork()
      */
 
-    virSecurityManagerPostFork(h->driver->securityManager);
+    qemuSecurityPostFork(h->driver->securityManager);
 
     /* Some later calls want pid present */
     h->vm->pid = getpid();
@@ -2651,7 +2648,7 @@ static int qemuProcessHook(void *data)
      * sockets the lock driver opens that we don't want
      * labelled. So far we're ok though.
      */
-    if (virSecurityManagerSetSocketLabel(h->driver->securityManager, h->vm->def) < 0)
+    if (qemuSecuritySetSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
     if (virDomainLockProcessStart(h->driver->lockManager,
                                   h->cfg->uri,
@@ -2660,7 +2657,7 @@ static int qemuProcessHook(void *data)
                                   true,
                                   &fd) < 0)
         goto cleanup;
-    if (virSecurityManagerClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
+    if (qemuSecurityClearSocketLabel(h->driver->securityManager, h->vm->def) < 0)
         goto cleanup;
 
     if (qemuDomainBuildNamespace(h->driver, h->vm) < 0)
@@ -3260,8 +3257,8 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver,
                     goto cleanup;
                 }
 
-                if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
-                                                         vm->def, hugepagePath) < 0) {
+                if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+                                                   vm->def, hugepagePath) < 0) {
                     virReportError(VIR_ERR_INTERNAL_ERROR,
                                    "%s", _("Unable to set huge path in security driver"));
                     goto cleanup;
@@ -3437,13 +3434,13 @@ qemuProcessReconnect(void *opaque)
     /* if domain requests security driver we haven't loaded, report error, but
      * do not kill the domain
      */
-    ignore_value(virSecurityManagerCheckAllLabel(driver->securityManager,
-                                                 obj->def));
+    ignore_value(qemuSecurityCheckAllLabel(driver->securityManager,
+                                           obj->def));
 
     if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0)
         goto error;
 
-    if (virSecurityManagerReserveLabel(driver->securityManager, obj->def, obj->pid) < 0)
+    if (qemuSecurityReserveLabel(driver->securityManager, obj->def, obj->pid) < 0)
         goto error;
 
     if (qemuProcessNotifyNets(obj->def) < 0)
@@ -4451,8 +4448,8 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
-                                             vm->def, path) < 0)
+    if (qemuSecurityDomainSetPathLabel(driver->securityManager,
+                                       vm->def, path) < 0)
         goto cleanup;
 
     ret = 0;
@@ -4647,7 +4644,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
         }
 
         VIR_DEBUG("Checking domain and device security labels");
-        if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0)
+        if (qemuSecurityCheckAllLabel(driver->securityManager, vm->def) < 0)
             return -1;
 
     }
@@ -5202,7 +5199,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
         /* If you are using a SecurityDriver with dynamic labelling,
            then generate a security label for isolation */
         VIR_DEBUG("Generating domain security label (if required)");
-        if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) {
+        if (qemuSecurityGenLabel(driver->securityManager, vm->def) < 0) {
             virDomainAuditSecurityLabel(vm, false);
             goto cleanup;
         }
@@ -5513,8 +5510,8 @@ qemuProcessLaunch(virConnectPtr conn,
     virCommandSetUmask(cmd, 0x002);
 
     VIR_DEBUG("Setting up security labelling");
-    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
-                                               vm->def, cmd) < 0)
+    if (qemuSecuritySetChildProcessLabel(driver->securityManager,
+                                         vm->def, cmd) < 0)
         goto cleanup;
 
     virCommandSetOutputFD(cmd, &logfile);
@@ -5524,10 +5521,10 @@ qemuProcessLaunch(virConnectPtr conn,
     virCommandDaemonize(cmd);
     virCommandRequireHandshake(cmd);
 
-    if (virSecurityManagerPreFork(driver->securityManager) < 0)
+    if (qemuSecurityPreFork(driver->securityManager) < 0)
         goto cleanup;
     rv = virCommandRun(cmd, NULL);
-    virSecurityManagerPostFork(driver->securityManager);
+    qemuSecurityPostFork(driver->securityManager);
 
     /* wait for qemu process to show up */
     if (rv == 0) {
@@ -5604,8 +5601,8 @@ qemuProcessLaunch(virConnectPtr conn,
             goto cleanup;
         }
         if (S_ISFIFO(stdin_sb.st_mode) &&
-            virSecurityManagerSetImageFDLabel(driver->securityManager,
-                                              vm->def, incoming->fd) < 0)
+            qemuSecuritySetImageFDLabel(driver->securityManager,
+                                        vm->def, incoming->fd) < 0)
             goto cleanup;
     }
 
@@ -6122,7 +6119,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
         qemuSecurityRestoreAllLabel(driver, vm,
                                     !!(flags & VIR_QEMU_PROCESS_STOP_MIGRATED));
 
-    virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+    qemuSecurityReleaseLabel(driver->securityManager, vm->def);
 
     for (i = 0; i < vm->def->ndisks; i++) {
         virDomainDeviceDef dev;
@@ -6366,13 +6363,13 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
     vm->pid = pid;
 
     VIR_DEBUG("Detect security driver config");
-    sec_managers = virSecurityManagerGetNested(driver->securityManager);
+    sec_managers = qemuSecurityGetNested(driver->securityManager);
     if (sec_managers == NULL)
         goto error;
 
     for (i = 0; sec_managers[i]; i++) {
         seclabelgen = false;
-        model = virSecurityManagerGetModel(sec_managers[i]);
+        model = qemuSecurityGetModel(sec_managers[i]);
         seclabeldef = virDomainDefGetSecurityLabelDef(vm->def, model);
         if (seclabeldef == NULL) {
             if (!(seclabeldef = virSecurityLabelDefNew(model)))
@@ -6382,8 +6379,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
         seclabeldef->type = VIR_DOMAIN_SECLABEL_STATIC;
         if (VIR_ALLOC(seclabel) < 0)
             goto error;
-        if (virSecurityManagerGetProcessLabel(sec_managers[i],
-                                              vm->def, vm->pid, seclabel) < 0)
+        if (qemuSecurityGetProcessLabel(sec_managers[i], vm->def,
+                                        vm->pid, seclabel) < 0)
             goto error;
 
         if (VIR_STRDUP(seclabeldef->model, model) < 0)
@@ -6400,9 +6397,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
         }
     }
 
-    if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0)
+    if (qemuSecurityCheckAllLabel(driver->securityManager, vm->def) < 0)
         goto error;
-    if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0)
+    if (qemuSecurityGenLabel(driver->securityManager, vm->def) < 0)
         goto error;
 
     if (qemuDomainPerfRestart(vm) < 0)
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index 54638908d..d86db3f6b 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -28,6 +28,7 @@
 
 # include "qemu_conf.h"
 # include "domain_conf.h"
+# include "security/security_manager.h"
 
 int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
                             virDomainObjPtr vm,
@@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
 int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
                                     virDomainObjPtr vm,
                                     virDomainHostdevDefPtr hostdev);
+
+/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
+ * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
+ */
+# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
+# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
+# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
+# define qemuSecurityGenLabel virSecurityManagerGenLabel
+# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
+# define qemuSecurityGetDOI virSecurityManagerGetDOI
+# define qemuSecurityGetModel virSecurityManagerGetModel
+# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
+# define qemuSecurityGetNested virSecurityManagerGetNested
+# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
+# define qemuSecurityNew virSecurityManagerNew
+# define qemuSecurityNewDAC virSecurityManagerNewDAC
+# define qemuSecurityNewStack virSecurityManagerNewStack
+# define qemuSecurityPostFork virSecurityManagerPostFork
+# define qemuSecurityPreFork virSecurityManagerPreFork
+# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
+# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
+# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
+# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
+# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
+# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
+# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
+# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
+# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
+# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
+# define qemuSecurityVerify virSecurityManagerVerify
+
 #endif /* __QEMU_SECURITY_H__ */
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Peter Krempa 7 years, 1 month ago
On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
> Now that we have some qemuSecurity wrappers over
> virSecurityManager APIs, lets make sure everybody sticks with
> them. We have them for a reason and calling virSecurityManager
> API directly instead of wrapper may lead into accidentally
> labelling a file on the host instead of namespace.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> This is an alternative approach to:
> 
> https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
> 
>  cfg.mk                    |  5 ++++
>  src/qemu/qemu_command.c   |  7 +++---
>  src/qemu/qemu_conf.c      |  9 ++++---
>  src/qemu/qemu_domain.c    | 17 ++++++-------
>  src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
>  src/qemu/qemu_hotplug.c   |  4 +--
>  src/qemu/qemu_migration.c | 13 +++++-----
>  src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
>  src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
>  9 files changed, 122 insertions(+), 89 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
> index 54638908d..d86db3f6b 100644
> --- a/src/qemu/qemu_security.h
> +++ b/src/qemu/qemu_security.h
> @@ -28,6 +28,7 @@
>  
>  # include "qemu_conf.h"
>  # include "domain_conf.h"
> +# include "security/security_manager.h"
>  
>  int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>                              virDomainObjPtr vm,
> @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
>  int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
>                                      virDomainObjPtr vm,
>                                      virDomainHostdevDefPtr hostdev);
> +
> +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
> + * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
> + */
> +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
> +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
> +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
> +# define qemuSecurityGenLabel virSecurityManagerGenLabel
> +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
> +# define qemuSecurityGetDOI virSecurityManagerGetDOI
> +# define qemuSecurityGetModel virSecurityManagerGetModel
> +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
> +# define qemuSecurityGetNested virSecurityManagerGetNested
> +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
> +# define qemuSecurityNew virSecurityManagerNew
> +# define qemuSecurityNewDAC virSecurityManagerNewDAC
> +# define qemuSecurityNewStack virSecurityManagerNewStack
> +# define qemuSecurityPostFork virSecurityManagerPostFork
> +# define qemuSecurityPreFork virSecurityManagerPreFork
> +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
> +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
> +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
> +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
> +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
> +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
> +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
> +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
> +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
> +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
> +# define qemuSecurityVerify virSecurityManagerVerify

I don't like this either for similar reasons that I've stated on the
original series.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Daniel P. Berrange 7 years, 1 month ago
On Mon, Feb 20, 2017 at 12:50:23PM +0100, Peter Krempa wrote:
> On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
> > Now that we have some qemuSecurity wrappers over
> > virSecurityManager APIs, lets make sure everybody sticks with
> > them. We have them for a reason and calling virSecurityManager
> > API directly instead of wrapper may lead into accidentally
> > labelling a file on the host instead of namespace.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > 
> > This is an alternative approach to:
> > 
> > https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
> > 
> >  cfg.mk                    |  5 ++++
> >  src/qemu/qemu_command.c   |  7 +++---
> >  src/qemu/qemu_conf.c      |  9 ++++---
> >  src/qemu/qemu_domain.c    | 17 ++++++-------
> >  src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
> >  src/qemu/qemu_hotplug.c   |  4 +--
> >  src/qemu/qemu_migration.c | 13 +++++-----
> >  src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
> >  src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
> >  9 files changed, 122 insertions(+), 89 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
> > index 54638908d..d86db3f6b 100644
> > --- a/src/qemu/qemu_security.h
> > +++ b/src/qemu/qemu_security.h
> > @@ -28,6 +28,7 @@
> >  
> >  # include "qemu_conf.h"
> >  # include "domain_conf.h"
> > +# include "security/security_manager.h"
> >  
> >  int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> >                              virDomainObjPtr vm,
> > @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
> >  int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
> >                                      virDomainObjPtr vm,
> >                                      virDomainHostdevDefPtr hostdev);
> > +
> > +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
> > + * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
> > + */
> > +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
> > +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
> > +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
> > +# define qemuSecurityGenLabel virSecurityManagerGenLabel
> > +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
> > +# define qemuSecurityGetDOI virSecurityManagerGetDOI
> > +# define qemuSecurityGetModel virSecurityManagerGetModel
> > +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
> > +# define qemuSecurityGetNested virSecurityManagerGetNested
> > +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
> > +# define qemuSecurityNew virSecurityManagerNew
> > +# define qemuSecurityNewDAC virSecurityManagerNewDAC
> > +# define qemuSecurityNewStack virSecurityManagerNewStack
> > +# define qemuSecurityPostFork virSecurityManagerPostFork
> > +# define qemuSecurityPreFork virSecurityManagerPreFork
> > +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
> > +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
> > +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
> > +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
> > +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
> > +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
> > +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
> > +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
> > +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
> > +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
> > +# define qemuSecurityVerify virSecurityManagerVerify
> 
> I don't like this either for similar reasons that I've stated on the
> original series.

I think the interesting question here is whether we can figure out a
way to push the qemuSecurity* functionality into the security drivers
and thus remove the need for all these wrappers....

Currently all the wrappers look the same doing

  if (namespace enabled) {
     ...start transaction...
  }
  ... call real method...
  if (namespace enabled) {
     ...commit transaction...
  }

The key observation to me is that there is only ever a single method
called inside the transaction. That ought to mean we can push the
transaction functionality down a layer. eg in the virSecurityManagerNew*
methods we could pass in a "bool useTransactions" flag and then the
security_manager.c could take care of start/commit/rollback

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Martin Kletzander 7 years, 1 month ago
On Mon, Feb 20, 2017 at 11:55:06AM +0000, Daniel P. Berrange wrote:
>On Mon, Feb 20, 2017 at 12:50:23PM +0100, Peter Krempa wrote:
>> On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
>> > Now that we have some qemuSecurity wrappers over
>> > virSecurityManager APIs, lets make sure everybody sticks with
>> > them. We have them for a reason and calling virSecurityManager
>> > API directly instead of wrapper may lead into accidentally
>> > labelling a file on the host instead of namespace.
>> >
>> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> > ---
>> >
>> > This is an alternative approach to:
>> >
>> > https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
>> >
>> >  cfg.mk                    |  5 ++++
>> >  src/qemu/qemu_command.c   |  7 +++---
>> >  src/qemu/qemu_conf.c      |  9 ++++---
>> >  src/qemu/qemu_domain.c    | 17 ++++++-------
>> >  src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
>> >  src/qemu/qemu_hotplug.c   |  4 +--
>> >  src/qemu/qemu_migration.c | 13 +++++-----
>> >  src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
>> >  src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
>> >  9 files changed, 122 insertions(+), 89 deletions(-)
>> >
>>
>> [...]
>>
>> > diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
>> > index 54638908d..d86db3f6b 100644
>> > --- a/src/qemu/qemu_security.h
>> > +++ b/src/qemu/qemu_security.h
>> > @@ -28,6 +28,7 @@
>> >
>> >  # include "qemu_conf.h"
>> >  # include "domain_conf.h"
>> > +# include "security/security_manager.h"
>> >
>> >  int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> >                              virDomainObjPtr vm,
>> > @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
>> >  int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
>> >                                      virDomainObjPtr vm,
>> >                                      virDomainHostdevDefPtr hostdev);
>> > +
>> > +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
>> > + * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
>> > + */
>> > +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
>> > +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
>> > +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
>> > +# define qemuSecurityGenLabel virSecurityManagerGenLabel
>> > +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
>> > +# define qemuSecurityGetDOI virSecurityManagerGetDOI
>> > +# define qemuSecurityGetModel virSecurityManagerGetModel
>> > +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
>> > +# define qemuSecurityGetNested virSecurityManagerGetNested
>> > +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
>> > +# define qemuSecurityNew virSecurityManagerNew
>> > +# define qemuSecurityNewDAC virSecurityManagerNewDAC
>> > +# define qemuSecurityNewStack virSecurityManagerNewStack
>> > +# define qemuSecurityPostFork virSecurityManagerPostFork
>> > +# define qemuSecurityPreFork virSecurityManagerPreFork
>> > +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
>> > +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
>> > +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
>> > +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
>> > +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
>> > +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
>> > +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
>> > +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
>> > +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
>> > +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
>> > +# define qemuSecurityVerify virSecurityManagerVerify
>>
>> I don't like this either for similar reasons that I've stated on the
>> original series.
>
>I think the interesting question here is whether we can figure out a
>way to push the qemuSecurity* functionality into the security drivers
>and thus remove the need for all these wrappers....
>
>Currently all the wrappers look the same doing
>
>  if (namespace enabled) {
>     ...start transaction...
>  }
>  ... call real method...
>  if (namespace enabled) {
>     ...commit transaction...
>  }
>
>The key observation to me is that there is only ever a single method
>called inside the transaction. That ought to mean we can push the
>transaction functionality down a layer. eg in the virSecurityManagerNew*
>methods we could pass in a "bool useTransactions" flag and then the
>security_manager.c could take care of start/commit/rollback
>

Yeah, I was thinking about that too before, one of the ideas I had
before.  However, you need the bool to be there per-domain, not
per-driver, and I haven't found out how to automatically take care of
commit (and rollback).  You'd have to call that anyway.

>Regards,
>Daniel
>--
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Michal Privoznik 7 years, 1 month ago
On 20.02.2017 12:55, Daniel P. Berrange wrote:
> On Mon, Feb 20, 2017 at 12:50:23PM +0100, Peter Krempa wrote:
>> On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
>>> Now that we have some qemuSecurity wrappers over
>>> virSecurityManager APIs, lets make sure everybody sticks with
>>> them. We have them for a reason and calling virSecurityManager
>>> API directly instead of wrapper may lead into accidentally
>>> labelling a file on the host instead of namespace.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>
>>> This is an alternative approach to:
>>>
>>> https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
>>>
>>>  cfg.mk                    |  5 ++++
>>>  src/qemu/qemu_command.c   |  7 +++---
>>>  src/qemu/qemu_conf.c      |  9 ++++---
>>>  src/qemu/qemu_domain.c    | 17 ++++++-------
>>>  src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
>>>  src/qemu/qemu_hotplug.c   |  4 +--
>>>  src/qemu/qemu_migration.c | 13 +++++-----
>>>  src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
>>>  src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
>>>  9 files changed, 122 insertions(+), 89 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
>>> index 54638908d..d86db3f6b 100644
>>> --- a/src/qemu/qemu_security.h
>>> +++ b/src/qemu/qemu_security.h
>>> @@ -28,6 +28,7 @@
>>>  
>>>  # include "qemu_conf.h"
>>>  # include "domain_conf.h"
>>> +# include "security/security_manager.h"
>>>  
>>>  int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>>>                              virDomainObjPtr vm,
>>> @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
>>>  int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
>>>                                      virDomainObjPtr vm,
>>>                                      virDomainHostdevDefPtr hostdev);
>>> +
>>> +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
>>> + * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
>>> + */
>>> +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
>>> +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
>>> +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
>>> +# define qemuSecurityGenLabel virSecurityManagerGenLabel
>>> +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
>>> +# define qemuSecurityGetDOI virSecurityManagerGetDOI
>>> +# define qemuSecurityGetModel virSecurityManagerGetModel
>>> +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
>>> +# define qemuSecurityGetNested virSecurityManagerGetNested
>>> +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
>>> +# define qemuSecurityNew virSecurityManagerNew
>>> +# define qemuSecurityNewDAC virSecurityManagerNewDAC
>>> +# define qemuSecurityNewStack virSecurityManagerNewStack
>>> +# define qemuSecurityPostFork virSecurityManagerPostFork
>>> +# define qemuSecurityPreFork virSecurityManagerPreFork
>>> +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
>>> +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
>>> +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
>>> +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
>>> +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
>>> +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
>>> +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
>>> +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
>>> +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
>>> +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
>>> +# define qemuSecurityVerify virSecurityManagerVerify
>>
>> I don't like this either for similar reasons that I've stated on the
>> original series.
> 
> I think the interesting question here is whether we can figure out a
> way to push the qemuSecurity* functionality into the security drivers
> and thus remove the need for all these wrappers....
> 
> Currently all the wrappers look the same doing
> 
>   if (namespace enabled) {
>      ...start transaction...
>   }
>   ... call real method...
>   if (namespace enabled) {
>      ...commit transaction...
>   }
> 
> The key observation to me is that there is only ever a single method
> called inside the transaction. That ought to mean we can push the
> transaction functionality down a layer. eg in the virSecurityManagerNew*
> methods we could pass in a "bool useTransactions" flag and then the
> security_manager.c could take care of start/commit/rollback

That would not fly. As Martin pointed out we need to use transactions on
per-domain basis. The decision whether to use transactions is based on a
boolean stored in the domain object and not the domain definition (well,
technically it's not a boolean rather than bitmap, but that's not the
point here).
I thought of registering a callback in the secdriver which would return
true/false depending whether transactions should be used. But that would
not fly either - secdriver takes domain def and not domain obj. So it
can only feed the callback with domain def which lacks the information
whether namespaces are used or not.

The other solution I can come up with is to use transactions
unconditionally - even for domains that don't run in a namespace. The
only drawback there is useless fork to enter the namespace we are
already in.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Peter Krempa 7 years ago
On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
> Now that we have some qemuSecurity wrappers over
> virSecurityManager APIs, lets make sure everybody sticks with
> them. We have them for a reason and calling virSecurityManager
> API directly instead of wrapper may lead into accidentally
> labelling a file on the host instead of namespace.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> This is an alternative approach to:
> 
> https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html

While I think that by putting some more effor to the script checking the
rules it would be possible to achieve the same even without having to
have macros for the APIs which don't require wrapping I must agree that
it's better to have a check with techincal debt rather than a bug.

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers
Posted by Michal Privoznik 7 years ago
On 06.03.2017 12:43, Peter Krempa wrote:
> On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
>> Now that we have some qemuSecurity wrappers over
>> virSecurityManager APIs, lets make sure everybody sticks with
>> them. We have them for a reason and calling virSecurityManager
>> API directly instead of wrapper may lead into accidentally
>> labelling a file on the host instead of namespace.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> This is an alternative approach to:
>>
>> https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
> 
> While I think that by putting some more effor to the script checking the
> rules it would be possible to achieve the same even without having to
> have macros for the APIs which don't require wrapping I must agree that
> it's better to have a check with techincal debt rather than a bug.

Agreed. I believe we might turn some of those dummy #define-s into
actual functions one day. But at least for now we will not introduce new
bugs.

> 
> ACK
> 

Thank you, pushed. One thing less to remember while reviewing qemu patches.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list