[libvirt] [PATCH v2 0/4] Couple of seclabels improvements
diff to v1: - rework the 3rd patch - introduce one more bugfix Michal Privoznik (4): virSecurityLabelDef: substitute 'norelabel' with 'relabel' virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel' conf: Always format seclabel's model conf: Don't allow multiple seclabels for same model src/conf/domain_conf.c | 67 ++++++++++++---------- src/security/security_apparmor.c | 10 ++-- src/security/security_dac.c | 22 +++---- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 32 +++++------ src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 4 +- .../qemuxml2argv-seclabel-dynamic-none.xml | 28 +++++++++ .../qemuxml2argv-seclabel-multiple.xml | 40 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 142 insertions(+), 67 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml -- 1.8.5.5
This negation in names of boolean variables is driving me insane. The code is much more readable if we drop the 'no-' prefix. Well, at least for me. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++---------- src/security/security_apparmor.c | 10 +++++----- src/security/security_dac.c | 14 +++++++------- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 24 ++++++++++-------------- src/util/virseclabel.h | 2 +- 6 files changed, 34 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fa76eb4..8384b5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4576,9 +4576,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) { if (STREQ(p, "yes")) { - def->norelabel = false; + def->relabel = true; } else if (STREQ(p, "no")) { - def->norelabel = true; + def->relabel = false; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), p); @@ -4587,13 +4587,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } VIR_FREE(p); if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && - def->norelabel) { + !def->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dynamic label type must use resource relabeling")); goto error; } if (def->type == VIR_DOMAIN_SECLABEL_NONE && - !def->norelabel) { + def->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("resource relabeling is not compatible with 'none' label type")); goto error; @@ -4601,9 +4601,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } else { if (def->type == VIR_DOMAIN_SECLABEL_STATIC || def->type == VIR_DOMAIN_SECLABEL_NONE) - def->norelabel = true; + def->relabel = false; else - def->norelabel = false; + def->relabel = true; } /* Always parse model */ @@ -4635,7 +4635,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } /* Only parse imagelabel, if requested live XML with relabeling */ - if (!def->norelabel && + if (def->relabel && (!(flags & VIR_DOMAIN_XML_INACTIVE) && def->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./imagelabel[1])", @@ -4793,7 +4793,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, } /* Can't use overrides if top-level doesn't allow relabeling. */ - if (vmDef && vmDef->norelabel) { + if (vmDef && !vmDef->relabel) { virReportError(VIR_ERR_XML_ERROR, "%s", _("label overrides require relabeling to be " "enabled at the domain level")); @@ -14708,14 +14708,14 @@ virSecurityLabelDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, " relabel='%s'", - def->norelabel ? "no" : "yes"); + def->relabel ? "yes" : "no"); if (def->label || def->imagelabel || def->baselabel) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<label>%s</label>\n", def->label); - if (!def->norelabel) + if (def->relabel) virBufferEscapeString(buf, "<imagelabel>%s</imagelabel>\n", def->imagelabel); if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1e2a38b..9603c78 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -281,7 +281,7 @@ reload_profile(virSecurityManagerPtr mgr, if (!secdef) return rc; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if ((profile_name = get_profile_name(def)) == NULL) @@ -481,7 +481,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; /* Reload the profile if stdin_path is specified. Note that @@ -718,7 +718,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!(secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME))) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if (secdef->imagelabel) { @@ -805,7 +805,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) @@ -904,7 +904,7 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; return reload_profile(mgr, def, NULL, false); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 26cd615..665cbc9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -307,7 +307,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (secdef && secdef->norelabel) + if (secdef && !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -369,7 +369,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (secdef && secdef->norelabel) + if (secdef && !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -477,7 +477,7 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, cbdata.manager = mgr; cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (cbdata.secdef && cbdata.secdef->norelabel) + if (cbdata.secdef && !cbdata.secdef->relabel) return 0; switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { @@ -601,7 +601,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) @@ -881,7 +881,7 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; VIR_DEBUG("Restoring security label on %s migrated=%d", @@ -955,7 +955,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; for (i = 0; i < def->ndisks; i++) { @@ -1157,7 +1157,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, return rc; } - if (!seclabel->norelabel && !seclabel->imagelabel && + if (seclabel->relabel && !seclabel->imagelabel && VIR_STRDUP(seclabel->imagelabel, seclabel->label) < 0) { VIR_FREE(seclabel->label); return rc; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 16bec5c..8a45e04 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -616,7 +616,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; } else { seclabel->type = VIR_DOMAIN_SECLABEL_NONE; - seclabel->norelabel = true; + seclabel->relabel = false; } } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e06c003..8d4a9aa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1130,7 +1130,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (seclabel->norelabel || (disk_seclabel && disk_seclabel->norelabel)) + if (!seclabel->relabel || (disk_seclabel && disk_seclabel->norelabel)) return 0; /* If labelskip is true and there are no backing files, then we @@ -1202,7 +1202,7 @@ virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -1456,7 +1456,7 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; switch (dev->mode) { @@ -1641,7 +1641,7 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; switch (dev->mode) { @@ -1670,7 +1670,7 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || seclabel->norelabel) + if (!seclabel || !seclabel->relabel) return 0; if (dev) @@ -1741,7 +1741,7 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || seclabel->norelabel) + if (!seclabel || !seclabel->relabel) return 0; if (dev) @@ -1866,10 +1866,8 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, VIR_DEBUG("Restoring security label on %s", def->name); secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return 0; - if (secdef->norelabel || data->skipAllLabel) + if (!secdef || !secdef->relabel || data->skipAllLabel) return 0; if (def->tpm) { @@ -1956,7 +1954,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel); @@ -1971,7 +1969,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; return virSecuritySELinuxRestoreSecurityFileLabel(mgr, savefile); @@ -2245,10 +2243,8 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return 0; - if (secdef->norelabel || data->skipAllLabel) + if (!secdef || !secdef->relabel || data->skipAllLabel) return 0; for (i = 0; i < def->ndisks; i++) { diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index b90d212..8d671fd 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -40,7 +40,7 @@ struct _virSecurityLabelDef { char *imagelabel; /* security image label string */ char *baselabel; /* base name of label string */ int type; /* virDomainSeclabelType */ - bool norelabel; + bool relabel; /* should try labeling attempts? */ bool implicit; /* true if seclabel is auto-added */ }; -- 1.8.5.5
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
This negation in names of boolean variables is driving me insane. The code is much more readable if we drop the 'no-' prefix. Well, at least for me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++---------- src/security/security_apparmor.c | 10 +++++----- src/security/security_dac.c | 14 +++++++------- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 24 ++++++++++-------------- src/util/virseclabel.h | 2 +- 6 files changed, 34 insertions(+), 38 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 16bec5c..8a45e04 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -616,7 +616,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
seclabel->relabel = true; is needed here now, since the code was relying on norelabel being false by default to enable relabeling (and I agree with your comment about readability now :)) The new default also affects the other caller of virSecurityLabelDefNew: In qemuProcessAttach where we generate a new label: if (seclabeldef == NULL) { if (!(seclabeldef = virSecurityLabelDefNew(model))) goto error; seclabelgen = true; } I'd set relabel to true here, to make this commit a no-op.
} else { seclabel->type = VIR_DOMAIN_SECLABEL_NONE; - seclabel->norelabel = true; + seclabel->relabel = false; } }
diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index b90d212..8d671fd 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -40,7 +40,7 @@ struct _virSecurityLabelDef { char *imagelabel; /* security image label string */ char *baselabel; /* base name of label string */ int type; /* virDomainSeclabelType */ - bool norelabel; + bool relabel; /* should try labeling attempts? */
I can't parse that. How about "whether we relabel files", or just leaving it without a comment? ACK with the two callers of virSecurityLabelDefNew fixed. Jan
On 07/10/2014 09:02 AM, Ján Tomko wrote:
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
This negation in names of boolean variables is driving me insane. The code is much more readable if we drop the 'no-' prefix. Well, at least for me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+++ b/src/security/security_manager.c @@ -616,7 +616,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
seclabel->relabel = true; is needed here now, since the code was relying on norelabel being false by default to enable relabeling (and I agree with your comment about readability now :))
The new default also affects the other caller of virSecurityLabelDefNew: In qemuProcessAttach where we generate a new label:
if (seclabeldef == NULL) { if (!(seclabeldef = virSecurityLabelDefNew(model))) goto error; seclabelgen = true; }
I'd set relabel to true here, to make this commit a no-op.
Actually, that would argue that virSecurityLabelDefNew() (and any other allocation function) should set the relabel=true as PART of the allocation. Any time that we rely on a default state, if the state is all 0 then VIR_ALLOC is okay, and if the default state requires non-zero, then all clients should use an allocation function and the allocation function should take care of the default. For example, see the commit pair bc3f5f1 (where I ensured that all clients allocate via a common function) and c123ef7 (where I used that common allocation function to initialize a non-zero member, rather than having to touch each caller).
+++ b/src/util/virseclabel.h @@ -40,7 +40,7 @@ struct _virSecurityLabelDef { char *imagelabel; /* security image label string */ char *baselabel; /* base name of label string */ int type; /* virDomainSeclabelType */ - bool norelabel; + bool relabel; /* should try labeling attempts? */
I can't parse that. How about "whether we relabel files", or just leaving it without a comment?
or even "true (default) for allowing relabels" -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Similarly to the previous commit, boolean variables should not start with 'no-' prefix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ src/security/security_dac.c | 8 ++++---- src/security/security_selinux.c | 10 +++++----- src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8384b5d..eccecd4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4803,9 +4803,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { - seclabels[i]->norelabel = false; + seclabels[i]->relabel = true; } else if (STREQ(relabel, "no")) { - seclabels[i]->norelabel = true; + seclabels[i]->relabel = false; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), @@ -4815,7 +4815,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, } VIR_FREE(relabel); } else { - seclabels[i]->norelabel = false; + seclabels[i]->relabel = true; } /* labelskip is only parsed on live images */ @@ -4830,7 +4830,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); seclabels[i]->label = label; - if (label && seclabels[i]->norelabel) { + if (label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"), @@ -14736,7 +14736,7 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf, { /* For offline output, skip elements that allow labels but have no * label specified (possible if labelskip was ignored on input). */ - if ((flags & VIR_DOMAIN_XML_INACTIVE) && !def->label && !def->norelabel) + if ((flags & VIR_DOMAIN_XML_INACTIVE) && !def->label && def->relabel) return; virBufferAddLit(buf, "<seclabel"); @@ -14747,7 +14747,7 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf, if (def->labelskip) virBufferAddLit(buf, " labelskip='yes'"); else - virBufferAsprintf(buf, " relabel='%s'", def->norelabel ? "no" : "yes"); + virBufferAsprintf(buf, " relabel='%s'", def->relabel ? "yes" : "no"); if (def->label) { virBufferAddLit(buf, ">\n"); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 665cbc9..4d2a9d6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -312,7 +312,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; if (disk_seclabel && disk_seclabel->label) { @@ -374,7 +374,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; /* If we have a shared FS and are doing migration, we must not change @@ -703,7 +703,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_DAC_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; if (chr_seclabel && chr_seclabel->label) { @@ -772,7 +772,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_DAC_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; switch ((virDomainChrType) dev_source->type) { diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 8d4a9aa..a0e89b7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1130,7 +1130,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (!seclabel->relabel || (disk_seclabel && disk_seclabel->norelabel)) + if (!seclabel->relabel || (disk_seclabel && !disk_seclabel->relabel)) return 0; /* If labelskip is true and there are no backing files, then we @@ -1208,10 +1208,10 @@ virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; - if (disk_seclabel && !disk_seclabel->norelabel && disk_seclabel->label) { + if (disk_seclabel && disk_seclabel->relabel && disk_seclabel->label) { ret = virSecuritySELinuxSetFilecon(src->path, disk_seclabel->label); } else if (first) { if (src->shared) { @@ -1677,7 +1677,7 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; if (chr_seclabel) @@ -1747,7 +1747,7 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, if (dev) chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; switch (dev_source->type) { diff --git a/src/util/virseclabel.c b/src/util/virseclabel.c index 8f07de3..88c1fc3 100644 --- a/src/util/virseclabel.c +++ b/src/util/virseclabel.c @@ -90,7 +90,7 @@ virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) if (VIR_ALLOC(ret) < 0) return NULL; - ret->norelabel = src->norelabel; + ret->relabel = src->relabel; ret->labelskip = src->labelskip; if (VIR_STRDUP(ret->model, src->model) < 0 || diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 8d671fd..7eebd60 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -51,7 +51,7 @@ typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; struct _virSecurityDeviceLabelDef { char *model; char *label; /* image label string */ - bool norelabel; /* true to skip label attempts */ + bool relabel; /* should try labeling attempts? */ bool labelskip; /* live-only; true if skipping failed label attempt */ }; -- 1.8.5.5
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
Similarly to the previous commit, boolean variables should not start with 'no-' prefix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ src/security/security_dac.c | 8 ++++---- src/security/security_selinux.c | 10 +++++----- src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 8d671fd..7eebd60 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -51,7 +51,7 @@ typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; struct _virSecurityDeviceLabelDef { char *model; char *label; /* image label string */ - bool norelabel; /* true to skip label attempts */ + bool relabel; /* should try labeling attempts? */
Same issue as in the first patch.
bool labelskip; /* live-only; true if skipping failed label attempt */ };
ACK Jan
https://bugzilla.redhat.com/show_bug.cgi?id=1113860 We've always done that. Well, until 990e46c45. Point is, if we don't format model, we may lose a domain on libvirtd restart. If the seclabel is implicit however, we should skip it's formatting. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 17 ++++++------- .../qemuxml2argv-seclabel-dynamic-none.xml | 28 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 3 files changed, 36 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eccecd4..c730d37 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14677,8 +14677,7 @@ virDomainEventActionDefFormat(virBufferPtr buf, static void virSecurityLabelDefFormat(virBufferPtr buf, - virSecurityLabelDefPtr def, - unsigned flags) + virSecurityLabelDefPtr def) { const char *sectype = virDomainSeclabelTypeToString(def->type); @@ -14688,19 +14687,17 @@ virSecurityLabelDefFormat(virBufferPtr buf, if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT) return; - /* To avoid backward compatibility issues, suppress DAC labels that are - * automatically generated. + /* To avoid backward compatibility issues, suppress DAC and 'none' labels + * that are automatically generated. */ - if (STREQ_NULLABLE(def->model, "dac") && def->implicit) + if ((STREQ_NULLABLE(def->model, "dac") || + STREQ_NULLABLE(def->model, "none")) && def->implicit) return; virBufferAsprintf(buf, "<seclabel type='%s'", sectype); - /* When generating state XML do include the model */ - if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS || - STRNEQ_NULLABLE(def->model, "none")) - virBufferEscapeString(buf, " model='%s'", def->model); + virBufferEscapeString(buf, " model='%s'", def->model); if (def->type == VIR_DOMAIN_SECLABEL_NONE) { virBufferAddLit(buf, "/>\n"); @@ -17910,7 +17907,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</devices>\n"); for (n = 0; n < def->nseclabels; n++) - virSecurityLabelDefFormat(buf, def->seclabels[n], flags); + virSecurityLabelDefFormat(buf, def->seclabels[n]); if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml new file mode 100644 index 0000000..7949f99 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='none' relabel='yes'/> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 26e3cad..9f919de 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -307,6 +307,7 @@ mymain(void) DO_TEST_FULL("seclabel-static-labelskip", false, WHEN_ACTIVE); DO_TEST("seclabel-none"); DO_TEST("seclabel-dac-none"); + DO_TEST("seclabel-dynamic-none"); DO_TEST("numad-static-vcpu-no-numatune"); DO_TEST("disk-scsi-lun-passthrough-sgio"); -- 1.8.5.5
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1113860
We've always done that. Well, until 990e46c45. Point is, if we don't format model, we may lose a domain on libvirtd restart. If the seclabel is implicit however, we should skip it's formatting.
It seems we only generate a seclabel with model="none" with type="none". Specifying other types (or the relabel attribute) with model="none" doesn't make much sense. Could we instead error out on other types than "none" when model is "none", or (if we can't) silently correct it to type "none"? Also, formatting model="none" in the status XML shouldn't be needed after that since we only generate it with type="none" and we would reject other types. Jan
https://bugzilla.redhat.com/show_bug.cgi?id=1066894 With current code it's possible to have for instance: virsh dumpxml mydomain | grep seclabel <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> what doesn't make any sense. We should reject the XML in the config parsing phase. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 18 ++++++++-- .../qemuxml2argv-seclabel-multiple.xml | 40 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c730d37..ace3ddf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4668,7 +4668,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, virCapsPtr caps, unsigned int flags) { - size_t i = 0; + size_t i = 0, j; int n; xmlNodePtr *list = NULL, saved_node; virCapsHostPtr host = &caps->host; @@ -4689,10 +4689,22 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, /* Parse each "seclabel" tag */ for (i = 0; i < n; i++) { + virSecurityLabelDefPtr seclabel; + ctxt->node = list[i]; - def->seclabels[i] = virSecurityLabelDefParseXML(ctxt, flags); - if (def->seclabels[i] == NULL) + if (!(seclabel = virSecurityLabelDefParseXML(ctxt, flags))) goto error; + + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) { + virReportError(VIR_ERR_XML_DETAIL, + _("seclablel for model %s is already provided"), + seclabel->model); + goto error; + } + } + + def->seclabels[i] = seclabel; } def->nseclabels = n; ctxt->node = saved_node; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml new file mode 100644 index 0000000..bd6fd15 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml @@ -0,0 +1,40 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'> + <seclabel model='selinux' labelskip='yes'/> + </source> + <backingStore/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='none' relabel='no'/> + <seclabel type='dynamic' model='dac' relabel='yes'/> + <seclabel type='static' model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + <imagelabel>system_u:system_r:svirt_custom_t:s0:c192,c392</imagelabel> + </seclabel> + <seclabel type='static' model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c393</label> + <imagelabel>system_u:system_r:svirt_custom_t:s0:c192,c393</imagelabel> + </seclabel> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bbc0fb7..a841adb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1223,6 +1223,7 @@ mymain(void) DO_TEST("seclabel-static-labelskip", QEMU_CAPS_NAME); DO_TEST("seclabel-none", QEMU_CAPS_NAME); DO_TEST("seclabel-dac-none", QEMU_CAPS_NAME); + DO_TEST_PARSE_ERROR("seclabel-multiple", QEMU_CAPS_NAME); DO_TEST("pseries-basic", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); -- 1.8.5.5
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1066894
With current code it's possible to have for instance:
virsh dumpxml mydomain | grep seclabel <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/> <seclabel type='dynamic' model='selinux' relabel='yes'/>
what doesn't make any sense. We should reject the XML in the config
s/what/which/
parsing phase.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 18 ++++++++-- .../qemuxml2argv-seclabel-multiple.xml | 40 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
@@ -4689,10 +4689,22 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
/* Parse each "seclabel" tag */ for (i = 0; i < n; i++) { + virSecurityLabelDefPtr seclabel; + ctxt->node = list[i]; - def->seclabels[i] = virSecurityLabelDefParseXML(ctxt, flags); - if (def->seclabels[i] == NULL) + if (!(seclabel = virSecurityLabelDefParseXML(ctxt, flags))) goto error; + + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) { + virReportError(VIR_ERR_XML_DETAIL, + _("seclablel for model %s is already provided"), + seclabel->model);
virSecurityLabelDefFree(seclabel);
+ goto error; + } + } + + def->seclabels[i] = seclabel; } def->nseclabels = n; ctxt->node = saved_node;
ACK with the leak fixed. Jan
On 07/10/2014 10:04 AM, Michal Privoznik wrote:
diff to v1: - rework the 3rd patch - introduce one more bugfix
Michal Privoznik (4): virSecurityLabelDef: substitute 'norelabel' with 'relabel' virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel' conf: Always format seclabel's model conf: Don't allow multiple seclabels for same model
src/conf/domain_conf.c | 67 ++++++++++++---------- src/security/security_apparmor.c | 10 ++-- src/security/security_dac.c | 22 +++---- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 32 +++++------ src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 4 +- .../qemuxml2argv-seclabel-dynamic-none.xml | 28 +++++++++ .../qemuxml2argv-seclabel-multiple.xml | 40 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 142 insertions(+), 67 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
There's a Coverity issue from these patches - it looks like perhaps patch 1&2 were combined when submitted into commit id '13adf1b' which has: virSecurityLabelDefPtr virSecurityLabelDefNew(const char *model) { virSecurityLabelDefPtr seclabel = NULL; if (VIR_ALLOC(seclabel) < 0 || VIR_STRDUP(seclabel->model, model) < 0) { virSecurityLabelDefFree(seclabel); seclabel = NULL; } + seclabel->relabel = true; + return seclabel; } See the problem at all? It's a FORWARD_NULL on 'seclabel'. John
participants (4)
-
Eric Blake -
John Ferlan -
Ján Tomko -
Michal Privoznik