[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] PATCH: Fix crash / cleanup bugs in sVirt verification



If defining a VM without any <seclabel> element present, the new 
verification code  will crash as it deferences NULL, so we just need
to skip over that.

Second, if one does a roundtrip of a live VM config

  virsh dumpxml foo > foo.xml
  virsh define foo.xml

and then stop and start the guest, it will now fail complaining that the
security label is already defined. This is because the XML parser is 
mistakenly parsing the 'model' attribute even for VMs whose label is
defined to be dynamic.

This then exposed anothe bug in the qemudStartVMDaemon() method where it
would not properly cleanup after a failed launch, failing to reset the
dynamic allocated security label, and also failing to close a logfil
file descriptor.

This patch fixes all of these problems

Daniel

Index: src/domain_conf.c
===================================================================
RCS file: /data/cvs/libvirt/src/domain_conf.c,v
retrieving revision 1.74
diff -u -p -r1.74 domain_conf.c
--- src/domain_conf.c	3 Apr 2009 10:55:51 -0000	1.74
+++ src/domain_conf.c	3 Apr 2009 12:14:54 -0000
@@ -1859,15 +1859,6 @@ virSecurityLabelDefParseXML(virConnectPt
     if (virXPathNode(conn, "./seclabel", ctxt) == NULL)
         return 0;
 
-    p = virXPathStringLimit(conn, "string(./seclabel/@model)",
-                            VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
-    if (p == NULL) {
-        virDomainReportError(conn, VIR_ERR_XML_ERROR,
-                             "%s", _("missing security model"));
-        goto error;
-    }
-    def->seclabel.model = p;
-
     p = virXPathStringLimit(conn, "string(./seclabel/@type)",
                             VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
     if (p == NULL) {
@@ -1888,6 +1879,14 @@ virSecurityLabelDefParseXML(virConnectPt
      */
     if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC ||
         !(flags & VIR_DOMAIN_XML_INACTIVE)) {
+        p = virXPathStringLimit(conn, "string(./seclabel/@model)",
+                                VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
+        if (p == NULL) {
+            virDomainReportError(conn, VIR_ERR_XML_ERROR,
+                                 "%s", _("missing security model"));
+            goto error;
+        }
+        def->seclabel.model = p;
 
         p = virXPathStringLimit(conn, "string(./seclabel/label[1])",
                                 VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
@@ -1905,8 +1904,11 @@ virSecurityLabelDefParseXML(virConnectPt
         !(flags & VIR_DOMAIN_XML_INACTIVE)) {
         p = virXPathStringLimit(conn, "string(./seclabel/imagelabel[1])",
                                 VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
-        if (p == NULL)
+        if (p == NULL) {
+            virDomainReportError(conn, VIR_ERR_XML_ERROR,
+                                 _("security imagelabel is missing"));
             goto error;
+        }
         def->seclabel.imagelabel = p;
     }
 
Index: src/qemu_driver.c
===================================================================
RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
retrieving revision 1.224
diff -u -p -r1.224 qemu_driver.c
--- src/qemu_driver.c	3 Apr 2009 10:55:51 -0000	1.224
+++ src/qemu_driver.c	3 Apr 2009 12:14:54 -0000
@@ -332,7 +332,7 @@ qemudReconnectVMs(struct qemud_driver *d
         }
 
         if ((vm->logfile = qemudLogFD(NULL, driver->logDir, vm->def->name)) < 0)
-            return -1;
+            goto next_error;
 
         if (vm->def->id >= driver->nextvmid)
             driver->nextvmid = vm->def->id + 1;
@@ -344,9 +344,12 @@ next_error:
         /* we failed to reconnect the vm so remove it's traces */
         vm->def->id = -1;
         qemudRemoveDomainStatus(NULL, driver, vm);
-        virDomainDefFree(vm->def);
-        vm->def = vm->newDef;
-        vm->newDef = NULL;
+        /* Restore orignal def, if we'd loaded a live one */
+        if (vm->newDef) {
+            virDomainDefFree(vm->def);
+            vm->def = vm->newDef;
+            vm->newDef = NULL;
+        }
 next:
         virDomainObjUnlock(vm);
         if (status)
@@ -1319,14 +1322,6 @@ static int qemudStartVMDaemon(virConnect
     hookData.vm = vm;
     hookData.driver = driver;
 
-   /* If you are using a SecurityDriver with dynamic labelling,
-      then generate a security label for isolation */
-    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
-        driver->securityDriver &&
-        driver->securityDriver->domainGenSecurityLabel &&
-        driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
-        return -1;
-
     FD_ZERO(&keepfd);
 
     if (virDomainIsActive(vm)) {
@@ -1335,6 +1330,14 @@ static int qemudStartVMDaemon(virConnect
         return -1;
     }
 
+    /* If you are using a SecurityDriver with dynamic labelling,
+       then generate a security label for isolation */
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
+        driver->securityDriver &&
+        driver->securityDriver->domainGenSecurityLabel &&
+        driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
+        return -1;
+
     if (vm->def->graphics &&
         vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
         vm->def->graphics->data.vnc.autoport) {
@@ -1342,7 +1345,7 @@ static int qemudStartVMDaemon(virConnect
         if (port < 0) {
             qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                              "%s", _("Unable to find an unused VNC port"));
-            return -1;
+            goto cleanup;
         }
         vm->def->graphics->data.vnc.port = port;
     }
@@ -1351,17 +1354,17 @@ static int qemudStartVMDaemon(virConnect
         virReportSystemError(conn, errno,
                              _("cannot create log directory %s"),
                              driver->logDir);
-        return -1;
+        goto cleanup;
     }
 
     if((vm->logfile = qemudLogFD(conn, driver->logDir, vm->def->name)) < 0)
-        return -1;
+        goto cleanup;
 
     emulator = vm->def->emulator;
     if (!emulator)
         emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps);
     if (!emulator)
-        return -1;
+        goto cleanup;
 
     /* Make sure the binary we are about to try exec'ing exists.
      * Technically we could catch the exec() failure, but that's
@@ -1371,7 +1374,7 @@ static int qemudStartVMDaemon(virConnect
         virReportSystemError(conn, errno,
                              _("Cannot find QEMU binary %s"),
                              emulator);
-        return -1;
+        goto cleanup;
     }
 
     if (qemudExtractVersionInfo(emulator,
@@ -1380,21 +1383,17 @@ static int qemudStartVMDaemon(virConnect
         qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("Cannot determine QEMU argv syntax %s"),
                          emulator);
-        return -1;
+        goto cleanup;
     }
 
     if (qemuPrepareHostDevices(conn, vm->def) < 0)
-        return -1;
+        goto cleanup;
 
     vm->def->id = driver->nextvmid++;
     if (qemudBuildCommandLine(conn, driver, vm,
                               qemuCmdFlags, &argv, &progenv,
-                              &tapfds, &ntapfds, migrateFrom) < 0) {
-        close(vm->logfile);
-        vm->def->id = -1;
-        vm->logfile = -1;
-        return -1;
-    }
+                              &tapfds, &ntapfds, migrateFrom) < 0)
+        goto cleanup;
 
     tmp = progenv;
     while (*tmp) {
@@ -1457,12 +1456,8 @@ static int qemudStartVMDaemon(virConnect
                              "%s", _("Unable to daemonize QEMU process"));
             ret = -1;
         }
-    }
-
-    if (ret == 0) {
         vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING;
-    } else
-        vm->def->id = -1;
+    }
 
     for (i = 0 ; argv[i] ; i++)
         VIR_FREE(argv[i]);
@@ -1479,19 +1474,38 @@ static int qemudStartVMDaemon(virConnect
         VIR_FREE(tapfds);
     }
 
-    if (ret == 0) {
-        if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) ||
-            (qemudDetectVcpuPIDs(conn, vm) < 0) ||
-            (qemudInitCpus(conn, vm, migrateFrom) < 0) ||
-            (qemudInitPasswords(conn, driver, vm) < 0) ||
-            (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) ||
-            (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) {
-            qemudShutdownVMDaemon(conn, driver, vm);
-            return -1;
-        }
+    if (ret == -1)
+        goto cleanup;
+
+    if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) ||
+        (qemudDetectVcpuPIDs(conn, vm) < 0) ||
+        (qemudInitCpus(conn, vm, migrateFrom) < 0) ||
+        (qemudInitPasswords(conn, driver, vm) < 0) ||
+        (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) ||
+        (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) {
+        qemudShutdownVMDaemon(conn, driver, vm);
+        ret = -1;
+        /* No need for 'goto cleanup' now since qemudShutdownVMDaemon does enough */
     }
 
     return ret;
+
+cleanup:
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
+        VIR_FREE(vm->def->seclabel.model);
+        VIR_FREE(vm->def->seclabel.label);
+        VIR_FREE(vm->def->seclabel.imagelabel);
+    }
+    if (vm->def->graphics &&
+        vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+        vm->def->graphics->data.vnc.autoport)
+        vm->def->graphics->data.vnc.port = -1;
+    if (vm->logfile != -1) {
+        close(vm->logfile);
+        vm->logfile = -1;
+    }
+    vm->def->id = -1;
+    return -1;
 }
 
 
Index: src/security.c
===================================================================
RCS file: /data/cvs/libvirt/src/security.c,v
retrieving revision 1.3
diff -u -p -r1.3 security.c
--- src/security.c	3 Apr 2009 10:55:51 -0000	1.3
+++ src/security.c	3 Apr 2009 12:14:54 -0000
@@ -33,7 +33,8 @@ virSecurityDriverVerify(virConnectPtr co
     unsigned int i;
     const virSecurityLabelDefPtr secdef = &def->seclabel;
 
-    if (STREQ(secdef->model, "none"))
+    if (!secdef->model ||
+        STREQ(secdef->model, "none"))
         return 0;
 
     for (i = 0; security_drivers[i] != NULL ; i++) {
@@ -42,7 +43,7 @@ virSecurityDriverVerify(virConnectPtr co
         }
     }
     virSecurityReportError(conn, VIR_ERR_XML_ERROR,
-                           _("invalid security model"));
+                           _("invalid security model '%s'"), secdef->model);
     return -1;
 }
 
Index: src/security_selinux.c
===================================================================
RCS file: /data/cvs/libvirt/src/security_selinux.c,v
retrieving revision 1.5
diff -u -p -r1.5 security_selinux.c
--- src/security_selinux.c	3 Apr 2009 10:55:51 -0000	1.5
+++ src/security_selinux.c	3 Apr 2009 12:14:54 -0000
@@ -162,11 +162,12 @@ SELinuxGenSecurityLabel(virConnectPtr co
     char *scontext = NULL;
     int c1 = 0;
     int c2 = 0;
-    if ( ( vm->def->seclabel.label ) ||
-         ( vm->def->seclabel.model ) ||
-         ( vm->def->seclabel.imagelabel )) {
-        virSecurityReportError(conn, VIR_ERR_ERROR,
-                               "%s", _("security labellin already defined for VM"));
+
+    if (vm->def->seclabel.label ||
+        vm->def->seclabel.model ||
+        vm->def->seclabel.imagelabel) {
+        virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                               "%s", _("security label already defined for VM"));
         return rc;
     }
 
@@ -197,7 +198,7 @@ SELinuxGenSecurityLabel(virConnectPtr co
         goto err;
     }
     vm->def->seclabel.model = strdup(SECURITY_SELINUX_NAME);
-    if (! vm->def->seclabel.model) {
+    if (!vm->def->seclabel.model) {
         virReportOOMError(conn);
         goto err;
     }

   

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]