On Thu, Nov 19, 2009 at 12:11:24PM +0000, Daniel P. Berrange wrote:

Even though QEMU does not directly open the saved image when restoring, it must be correctly labelled to allow QEMU to read from it because labelling is passed around with open file descriptors.

The labelling should not allow writing to the saved image again, only reading.

* src/qemu/qemu_driver.c: Label the save image when restoring * src/security/security_driver.h: Add a virSecurityDomainSetSavedStateLabelRO method for labelling a saved image for restore * src/security/security_selinux.c: Implement labelling of RO save images for restore --- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++----------- src/security/security_driver.h | 5 +++++ src/security/security_selinux.c | 11 +++++++++++ 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a4a87ac..d500e14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3484,7 +3484,7 @@ static int qemudDomainSave(virDomainPtr dom,

if (driver->securityDriver && driver->securityDriver->domainRestoreSavedStateLabel && - driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) == -1) + driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1) goto endjob;

ret = 0; @@ -4036,6 +4036,11 @@ static int qemudDomainRestore(virConnectPtr conn, if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup;

+ if (driver->securityDriver && + driver->securityDriver->domainSetSavedStateLabelRO && + driver->securityDriver->domainSetSavedStateLabelRO(conn, vm, path) == -1) + goto endjob; + if (header.version == 2) { const char *intermediate_argv[3] = { NULL, "-dc", NULL }; const char *prog = qemudSaveCompressionTypeToString(header.compressed); @@ -4070,15 +4075,8 @@ static int qemudDomainRestore(virConnectPtr conn, close(intermediatefd); close(fd); fd = -1; - if (ret < 0) { - if (!vm->persistent) { - qemuDomainObjEndJob(vm); - virDomainRemoveInactive(&driver->domains, - vm); - vm = NULL; - } + if (ret < 0) goto endjob; - }

event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, @@ -4102,8 +4100,19 @@ static int qemudDomainRestore(virConnectPtr conn, ret = 0;

endjob: - if (vm) - qemuDomainObjEndJob(vm); + qemuDomainObjEndJob(vm); + if (driver->securityDriver && + driver->securityDriver->domainRestoreSavedStateLabel && + driver->securityDriver->domainRestoreSavedStateLabel(conn, vm, path) == -1) + VIR_WARN("Unable to restore labelling on %s", path); + + if (ret < 0) { + if (!vm->persistent) { + virDomainRemoveInactive(&driver->domains, + vm); + vm = NULL; + } + }

cleanup: virDomainDefFree(def); diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 5514962..5144976 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -45,7 +45,11 @@ typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr conn, virDomainObjPtr vm, const char *savefile); +typedef int (*virSecurityDomainSetSavedStateLabelRO) (virConnectPtr conn, + virDomainObjPtr vm, + const char *savefile); typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn, + virDomainObjPtr vm, const char *savefile); typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, virDomainObjPtr sec); @@ -77,6 +81,7 @@ struct _virSecurityDriver { virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel; + virSecurityDomainSetSavedStateLabelRO domainSetSavedStateLabelRO; virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel;

/* diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index bd838e6..49f6746 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -637,7 +637,17 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,

static int +SELinuxSetSavedStateLabelRO(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + const char *savefile) +{ + return SELinuxSetFilecon(conn, savefile, default_content_context); +} + + +static int SELinuxRestoreSavedStateLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, const char *savefile) { return SELinuxRestoreSecurityFileLabel(conn, savefile);

ACK

@@ -714,5 +724,6 @@ virSecurityDriver virSELinuxSecurityDriver = { .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel, .domainSetSavedStateLabel = SELinuxSetSavedStateLabel, + .domainSetSavedStateLabelRO = SELinuxSetSavedStateLabelRO, .domainRestoreSavedStateLabel = SELinuxRestoreSavedStateLabel, };

maybe one could take the opportunity to convert to the explicit complete initialization for the security driver too, as we did for hypervisor ones. 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/