[libvirt] PATCH: Fix crash / cleanup bugs in sVirt verification
Daniel P. Berrange
berrange at redhat.com
Fri Apr 3 12:22:11 UTC 2009
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 :|
More information about the libvir-list
mailing list