[libvirt] [PATCH 0/4] Fix image labeling when saving a guest
This series cleans up a few places related to selinux labels and fixes issues with selinux when saving a machine with static selinux label and relabeling turned off. Peter Krempa (4): qemu: Improve info message and remove a variable in qemuDomainManagedSave conf: refactor virSecurityLabelDefParseXML security: Introduce method for labeling file descriptors of created files qemu: Always label newly created file on migration (save/managedsave) src/conf/domain_conf.c | 72 ++++++++++++++++++----------------------- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 7 ++-- src/qemu/qemu_migration.c | 4 +-- src/security/security_dac.c | 9 ++++++ src/security/security_driver.h | 4 +++ src/security/security_manager.c | 16 +++++++++ src/security/security_manager.h | 3 ++ src/security/security_nop.c | 1 + src/security/security_selinux.c | 21 ++++++++++++ src/security/security_stack.c | 19 +++++++++++ 11 files changed, 111 insertions(+), 46 deletions(-) -- 1.8.2.1
Mention the domain name that is being saved and remove the unneeded variable that only stores a constant. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f51e766..23eb4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3118,7 +3118,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *name = NULL; int ret = -1; - int compressed; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -3144,10 +3143,10 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; - VIR_INFO("Saving state to %s", name); + VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name); - compressed = QEMU_SAVE_FORMAT_RAW; - if ((ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, + if ((ret = qemuDomainSaveInternal(driver, dom, vm, name, + QEMU_SAVE_FORMAT_RAW, NULL, flags)) == 0) vm->hasManagedSave = true; -- 1.8.2.1
On Wed, Jun 26, 2013 at 03:01:47PM +0200, Peter Krempa wrote:
Mention the domain name that is being saved and remove the unneeded variable that only stores a constant. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f51e766..23eb4fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3118,7 +3118,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *name = NULL; int ret = -1; - int compressed;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -3144,10 +3143,10 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup;
- VIR_INFO("Saving state to %s", name); + VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name);
- compressed = QEMU_SAVE_FORMAT_RAW; - if ((ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, + if ((ret = qemuDomainSaveInternal(driver, dom, vm, name, + QEMU_SAVE_FORMAT_RAW, NULL, flags)) == 0) vm->hasManagedSave = true;
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/27/13 20:48, Daniel P. Berrange wrote:
On Wed, Jun 26, 2013 at 03:01:47PM +0200, Peter Krempa wrote:
Mention the domain name that is being saved and remove the unneeded variable that only stores a constant. --- src/qemu/qemu_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
...
ACK
Pushed. Thanks. PEter
Simplification of the code without functional impact. --- src/conf/domain_conf.c | 72 ++++++++++++++++++++++---------------------------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e41dfa2..6b6196c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4230,52 +4230,50 @@ static virSecurityLabelDefPtr virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, unsigned int flags) { - char *p; + char *str = NULL; virSecurityLabelDefPtr def = NULL; + size_t labellen = VIR_SECURITY_LABEL_BUFLEN - 1; if (VIR_ALLOC(def) < 0) { virReportOOMError(); goto error; } - p = virXPathStringLimit("string(./@type)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { + str = virXPathStringLimit("string(./@type)", labellen, ctxt); + if (!str) { def->type = VIR_DOMAIN_SECLABEL_DYNAMIC; } else { - def->type = virDomainSeclabelTypeFromString(p); - VIR_FREE(p); - if (def->type <= 0) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("invalid security type")); + if ((def->type = virDomainSeclabelTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid security type")); goto error; } } + VIR_FREE(str); - p = virXPathStringLimit("string(./@relabel)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p != NULL) { - if (STREQ(p, "yes")) { + if ((str = virXPathStringLimit("string(./@relabel)", labellen, ctxt))) { + if (STREQ(str, "yes")) { def->norelabel = false; - } else if (STREQ(p, "no")) { + } else if (STREQ(str, "no")) { def->norelabel = true; } else { virReportError(VIR_ERR_XML_ERROR, - _("invalid security relabel value %s"), p); - VIR_FREE(p); + _("invalid security relabel value %s"), str); goto error; } - VIR_FREE(p); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && def->norelabel) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("dynamic label type must use resource relabeling")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dynamic label type must use resource relabeling")); goto error; } + if (def->type == VIR_DOMAIN_SECLABEL_NONE && !def->norelabel) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("resource relabeling is not compatible with 'none' label type")); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("resource relabeling is not compatible with " + "'none' label type")); goto error; } } else { @@ -4285,6 +4283,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, else def->norelabel = false; } + VIR_FREE(str); /* Only parse label, if using static labels, or * if the 'live' VM XML is requested @@ -4292,46 +4291,39 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (def->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_XML_INACTIVE) && def->type != VIR_DOMAIN_SECLABEL_NONE)) { - p = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("security label is missing")); + if (!(def->label = virXPathStringLimit("string(./label[1])", + labellen, ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("security label is missing")); goto error; } - - def->label = p; } /* Only parse imagelabel, if requested live XML with relabeling */ if (!def->norelabel && (!(flags & VIR_DOMAIN_XML_INACTIVE) && def->type != VIR_DOMAIN_SECLABEL_NONE)) { - p = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("security imagelabel is missing")); + if (!(def->imagelabel = virXPathStringLimit("string(./imagelabel[1])", + labellen, ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("security imagelabel is missing")); goto error; } - def->imagelabel = p; } /* Only parse baselabel for dynamic label type */ if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - p = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - def->baselabel = p; + def->baselabel = virXPathStringLimit("string(./baselabel[1])", + labellen, ctxt); } /* Always parse model */ - p = virXPathStringLimit("string(./@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - def->model = p; + def->model = virXPathStringLimit("string(./@model)", labellen, ctxt); return def; error: + VIR_FREE(str); virSecurityLabelDefFree(def); return NULL; } -- 1.8.2.1
On Wed, Jun 26, 2013 at 03:01:48PM +0200, Peter Krempa wrote:
Simplification of the code without functional impact. --- src/conf/domain_conf.c | 72 ++++++++++++++++++++++---------------------------- 1 file changed, 32 insertions(+), 40 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e41dfa2..6b6196c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4230,52 +4230,50 @@ static virSecurityLabelDefPtr virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, unsigned int flags) { - char *p; + char *str = NULL; virSecurityLabelDefPtr def = NULL; + size_t labellen = VIR_SECURITY_LABEL_BUFLEN - 1;
/* Always parse model */ - p = virXPathStringLimit("string(./@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - def->model = p; + def->model = virXPathStringLimit("string(./@model)", labellen, ctxt);
labellen here does not match the original constant value. I don't like this change - I think it is clearer code as it was using the constant directly. The use of a variable obscures the code leading to bugs like the one you have introduced here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The method labels the file descriptor even if dynamic labeling/relabeling is turned off. This is needed for files created by libvirt and then passed along to qemu as a FD. --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 9 +++++++++ src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 16 ++++++++++++++++ src/security/security_manager.h | 3 +++ src/security/security_nop.c | 1 + src/security/security_selinux.c | 21 +++++++++++++++++++++ src/security/security_stack.c | 19 +++++++++++++++++++ 8 files changed, 74 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 795e011..dd06f11 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1035,6 +1035,7 @@ virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; +virSecurityManagerSetCreatedFDLabel; virSecurityManagerSetDaemonSocketLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetHugepages; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0d6defc..ef528f6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1180,6 +1180,14 @@ virSecurityDACSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, } static int +virSecurityDACSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int virSecurityDACSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, int fd ATTRIBUTE_UNUSED) @@ -1231,6 +1239,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainRestoreSavedStateLabel = virSecurityDACRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecurityDACSetImageFDLabel, + .domainSetSecurityCreatedFDLabel = virSecurityDACSetCreatedFDLabel, .domainSetSecurityTapFDLabel = virSecurityDACSetTapFDLabel, .domainGetSecurityMountOptions = virSecurityDACGetMountOptions, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cc401e1..0edcc34 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -100,6 +100,9 @@ typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); +typedef int (*virSecurityDomainSetCreatedFDLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + int fd); typedef int (*virSecurityDomainSetTapFDLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); @@ -146,6 +149,7 @@ struct _virSecurityDriver { virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; + virSecurityDomainSetCreatedFDLabel domainSetSecurityCreatedFDLabel; virSecurityDomainSetTapFDLabel domainSetSecurityTapFDLabel; virSecurityDomainGetMountOptions domainGetSecurityMountOptions; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f7c5c2e..2152246 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -663,6 +663,22 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, return -1; } +int virSecurityManagerSetCreatedFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + int fd) +{ + if (mgr->drv->domainSetSecurityCreatedFDLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityCreatedFDLabel(mgr, vm, fd); + virObjectUnlock(mgr); + return ret; + } + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; +} + int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, int fd) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 711b354..343dffb 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -112,6 +112,9 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); +int virSecurityManagerSetCreatedFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + int fd); int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, int fd); diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 233404c..ee0e05b 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -223,6 +223,7 @@ virSecurityDriver virSecurityDriverNop = { .domainRestoreSavedStateLabel = virSecurityDomainRestoreSavedStateLabelNop, .domainSetSecurityImageFDLabel = virSecurityDomainSetFDLabelNop, + .domainSetSecurityCreatedFDLabel = virSecurityDomainSetFDLabelNop, .domainSetSecurityTapFDLabel = virSecurityDomainSetFDLabelNop, .domainGetSecurityMountOptions = virSecurityDomainGetMountOptionsNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7802dda..5894259 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2446,6 +2446,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, return opts; } +static int +virSecuritySELinuxSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + int fd) +{ + virSecurityLabelDefPtr secdef; + + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { + if (!secdef->imagelabel) + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def); + } else { + return -1; + } + + if (secdef->imagelabel == NULL) + return 0; + + return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel); +} + virSecurityDriver virSecurityDriverSELinux = { .privateDataLen = sizeof(virSecuritySELinuxData), .name = SECURITY_SELINUX_NAME, @@ -2483,6 +2503,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainRestoreSavedStateLabel = virSecuritySELinuxRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecuritySELinuxSetImageFDLabel, + .domainSetSecurityCreatedFDLabel = virSecuritySELinuxSetCreatedFDLabel, .domainSetSecurityTapFDLabel = virSecuritySELinuxSetTapFDLabel, .domainGetSecurityMountOptions = virSecuritySELinuxGetSecurityMountOptions, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 14d757d..926ffbe 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -471,6 +471,24 @@ virSecurityStackSetImageFDLabel(virSecurityManagerPtr mgr, } static int +virSecurityStackSetCreatedFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + int fd) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetCreatedFDLabel(item->securityManager, vm, fd) < 0) + rc = -1; + } + + return rc; +} + + +static int virSecurityStackSetTapFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, int fd) @@ -569,6 +587,7 @@ virSecurityDriver virSecurityDriverStack = { .domainRestoreSavedStateLabel = virSecurityStackRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecurityStackSetImageFDLabel, + .domainSetSecurityCreatedFDLabel = virSecurityStackSetCreatedFDLabel, .domainSetSecurityTapFDLabel = virSecurityStackSetTapFDLabel, .domainGetSecurityMountOptions = virSecurityStackGetMountOptions, -- 1.8.2.1
On Wed, Jun 26, 2013 at 03:01:49PM +0200, Peter Krempa wrote:
The method labels the file descriptor even if dynamic labeling/relabeling is turned off. This is needed for files created by libvirt and then passed along to qemu as a FD. --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 9 +++++++++ src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 16 ++++++++++++++++ src/security/security_manager.h | 3 +++ src/security/security_nop.c | 1 + src/security/security_selinux.c | 21 +++++++++++++++++++++ src/security/security_stack.c | 19 +++++++++++++++++++ 8 files changed, 74 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7802dda..5894259 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2446,6 +2446,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, return opts; }
+static int +virSecuritySELinuxSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + int fd) +{ + virSecurityLabelDefPtr secdef; + + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { + if (!secdef->imagelabel) + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def);
This is really dubious. None of the methods except for GenSecurityLabel should be making changes to the secdef state.
+ } else { + return -1; + }
The style with nested if()s here is not following the pattern used in other methods here either.
+ + if (secdef->imagelabel == NULL) + return 0; + + return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel); +}
In general I'm not really convinced we should be adding a new method here, as opposed to making the existing SetImageFDLabel do the right thing. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/27/13 20:51, Daniel P. Berrange wrote:
On Wed, Jun 26, 2013 at 03:01:49PM +0200, Peter Krempa wrote:
The method labels the file descriptor even if dynamic labeling/relabeling is turned off. This is needed for files created by libvirt and then passed along to qemu as a FD. --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 9 +++++++++ src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 16 ++++++++++++++++ src/security/security_manager.h | 3 +++ src/security/security_nop.c | 1 + src/security/security_selinux.c | 21 +++++++++++++++++++++ src/security/security_stack.c | 19 +++++++++++++++++++ 8 files changed, 74 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7802dda..5894259 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2446,6 +2446,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, return opts; }
+static int +virSecuritySELinuxSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + int fd) +{ + virSecurityLabelDefPtr secdef; + + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { + if (!secdef->imagelabel) + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def);
This is really dubious. None of the methods except for GenSecurityLabel should be making changes to the secdef state.
There is already an exception: virSecuritySELinuxGetSecurityMountOptions().
+ } else { + return -1; + }
The style with nested if()s here is not following the pattern used in other methods here either.
hmmmm ... ok
+ + if (secdef->imagelabel == NULL) + return 0; + + return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel); +}
In general I'm not really convinced we should be adding a new method here, as opposed to making the existing SetImageFDLabel do the right thing.
I was going to suggest expanding the function prototype with a bool that would allow to choose whether to label the image always or only if relabeling is enabled but looking through the code I noticed that actually every caller would need to have this flag enabled. There are currently 4 places this method is called: qemuDumpToFd() - labels a FD of a freshly created file for the memory dump qemuMigrationToFile() - again a new file FD is labeled, used for (managed) save and dumping of memory doTunnelMigrate() - the FD's of a pipe used for extracting data into the stream are labeled, again this would probably fail with static labeling. qemuProcessStart() - the migration FD is labelled on process start. I think it's safe (and necessary) to relabel these even if relabeling is disabled and thus we can change the function to always label FD's. Do you agree?
Daniel
Peter
On Mon, Jul 01, 2013 at 02:28:46PM +0200, Peter Krempa wrote:
On 06/27/13 20:51, Daniel P. Berrange wrote:
On Wed, Jun 26, 2013 at 03:01:49PM +0200, Peter Krempa wrote:
The method labels the file descriptor even if dynamic labeling/relabeling is turned off. This is needed for files created by libvirt and then passed along to qemu as a FD. --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 9 +++++++++ src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 16 ++++++++++++++++ src/security/security_manager.h | 3 +++ src/security/security_nop.c | 1 + src/security/security_selinux.c | 21 +++++++++++++++++++++ src/security/security_stack.c | 19 +++++++++++++++++++ 8 files changed, 74 insertions(+)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7802dda..5894259 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2446,6 +2446,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr, return opts; }
+static int +virSecuritySELinuxSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + int fd) +{ + virSecurityLabelDefPtr secdef; + + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { + if (!secdef->imagelabel) + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def);
This is really dubious. None of the methods except for GenSecurityLabel should be making changes to the secdef state.
There is already an exception: virSecuritySELinuxGetSecurityMountOptions().
That is a not an example of good code & needs to be fixed for the same reason.
+ + if (secdef->imagelabel == NULL) + return 0; + + return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel); +}
In general I'm not really convinced we should be adding a new method here, as opposed to making the existing SetImageFDLabel do the right thing.
I was going to suggest expanding the function prototype with a bool that would allow to choose whether to label the image always or only if relabeling is enabled but looking through the code I noticed that actually every caller would need to have this flag enabled.
There are currently 4 places this method is called:
qemuDumpToFd() - labels a FD of a freshly created file for the memory dump
qemuMigrationToFile() - again a new file FD is labeled, used for (managed) save and dumping of memory
doTunnelMigrate() - the FD's of a pipe used for extracting data into the stream are labeled, again this would probably fail with static labeling.
qemuProcessStart() - the migration FD is labelled on process start.
I think it's safe (and necessary) to relabel these even if relabeling is disabled and thus we can change the function to always label FD's. Do you agree?
Yeah, it looks that way to me. All this relabel=no stuff was basically there to cope with files on NFS volumes. So the key thing is to make sure that with whatever change you make, the APIs above all work correctly if the file path points to something on NFS. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Migration to file when (managed)saving a machine failed when static labelling was used. Use the new security driver method to avoid this. --- src/qemu/qemu_migration.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 69d5398..e3203f6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4578,8 +4578,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 (virSecurityManagerSetCreatedFDLabel(driver->securityManager, vm->def, + compressor ? pipeFD[1] : fd) < 0) goto cleanup; bypassSecurityDriver = true; } else { -- 1.8.2.1
participants (2)
-
Daniel P. Berrange -
Peter Krempa