[libvirt] [PATCH] SELinux: don't silently fail when no label is present
This fixes startup of a domain with: <seclabel type='none' model='dac'/> on a host with selinux and dac drivers and security_default_confined = 0 https://bugzilla.redhat.com/show_bug.cgi?id=1105939 --- src/security/security_selinux.c | 98 ++++++++++++----------------------------- 1 file changed, 29 insertions(+), 69 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 8380bba..008c58c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -585,7 +585,7 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) - return rc; + return 0; data = virSecurityManagerGetPrivateData(mgr); @@ -739,11 +739,7 @@ virSecuritySELinuxReserveSecurityLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr seclabel; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (seclabel == NULL) { - return -1; - } - - if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) + if (!seclabel || seclabel->type == VIR_DOMAIN_SECLABEL_STATIC) return 0; if (getpidcon_raw(pid, &pctx) == -1) { @@ -1060,7 +1056,7 @@ virSecuritySELinuxSetSecurityTPMFileLabel(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) - return -1; + return 0; switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: @@ -1102,7 +1098,7 @@ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) - return -1; + return 0; switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: @@ -1136,7 +1132,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) - return -1; + return 0; disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, SECURITY_SELINUX_NAME); @@ -1256,10 +1252,7 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, cbdata.manager = mgr; cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (cbdata.secdef == NULL) - return -1; - - if (cbdata.secdef->norelabel) + if (!cbdata.secdef || cbdata.secdef->norelabel) return 0; if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) @@ -1279,7 +1272,7 @@ virSecuritySELinuxSetSecurityHostdevLabelHelper(const char *file, void *opaque) secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; return virSecuritySELinuxSetFilecon(file, secdef->imagelabel); } @@ -1397,7 +1390,7 @@ virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; switch (dev->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: { @@ -1447,10 +1440,7 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->norelabel) + if (!secdef || secdef->norelabel) return 0; switch (dev->mode) { @@ -1635,10 +1625,7 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->norelabel) + if (!secdef || secdef->norelabel) return 0; switch (dev->mode) { @@ -1667,14 +1654,14 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (seclabel == NULL) - return -1; + if (!seclabel || seclabel->norelabel) + return 0; if (dev) chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) + if (chr_seclabel && chr_seclabel->norelabel) return 0; if (chr_seclabel) @@ -1738,13 +1725,13 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (seclabel == NULL) - return -1; + if (!seclabel || seclabel->norelabel) + return 0; if (dev) chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (seclabel->norelabel || (chr_seclabel && chr_seclabel->norelabel)) + if (chr_seclabel && chr_seclabel->norelabel) return 0; switch (dev_source->type) { @@ -1864,7 +1851,7 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; if (secdef->norelabel || data->skipAllLabel) return 0; @@ -1925,7 +1912,7 @@ virSecuritySELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { if (secdef->label != NULL) { @@ -1953,10 +1940,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->norelabel) + if (!secdef || secdef->norelabel) return 0; return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel); @@ -1971,10 +1955,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->norelabel) + if (!secdef || secdef->norelabel) return 0; return virSecuritySELinuxRestoreSecurityFileLabel(mgr, savefile); @@ -1989,7 +1970,7 @@ virSecuritySELinuxSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; if (!STREQ(SECURITY_SELINUX_NAME, secdef->model)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2018,10 +1999,7 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; VIR_DEBUG("label=%s", secdef->label); @@ -2055,10 +2033,7 @@ virSecuritySELinuxSetSecurityChildProcessLabel(virSecurityManagerPtr mgr ATTRIBU virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; VIR_DEBUG("label=%s", secdef->label); @@ -2088,10 +2063,7 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr ATTRIBU int rc = -1; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; if (!STREQ(SECURITY_SELINUX_NAME, secdef->model)) { @@ -2138,10 +2110,7 @@ virSecuritySELinuxSetSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNU int rc = -1; secdef = virDomainDefGetSecurityLabelDef(vm, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; if (!STREQ(SECURITY_SELINUX_NAME, secdef->model)) { @@ -2179,10 +2148,7 @@ virSecuritySELinuxClearSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_U virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; if (!STREQ(SECURITY_SELINUX_NAME, secdef->model)) { @@ -2264,7 +2230,7 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - return -1; + return 0; if (secdef->norelabel || data->skipAllLabel) return 0; @@ -2337,10 +2303,7 @@ virSecuritySELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return -1; - - if (secdef->imagelabel == NULL) + if (!secdef || !secdef->imagelabel) return 0; return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel); @@ -2358,10 +2321,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, int rc = -1; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return rc; - - if (secdef->label == NULL) + if (!secdef || !secdef->label) return 0; if (fstat(fd, &buf) < 0) { -- 1.8.3.2
On 06/09/2014 08:30 AM, Ján Tomko wrote:
This fixes startup of a domain with: <seclabel type='none' model='dac'/> on a host with selinux and dac drivers and security_default_confined = 0
https://bugzilla.redhat.com/show_bug.cgi?id=1105939 --- src/security/security_selinux.c | 98 ++++++++++++----------------------------- 1 file changed, 29 insertions(+), 69 deletions(-)
Shouldn't there also be a change somewhere under tests/ to ensure that we parse XML in that situation? Otherwise, this looks like a fairly mechanical conversion of virDomainDefGetSecurityLabelDef() returning NULL from being a silent error into being successful; and that makes sense since domain_conf.c does not report any errors if a label cannot be looked up. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/10/2014 12:33 AM, Eric Blake wrote:
On 06/09/2014 08:30 AM, Ján Tomko wrote:
This fixes startup of a domain with: <seclabel type='none' model='dac'/> on a host with selinux and dac drivers and security_default_confined = 0
https://bugzilla.redhat.com/show_bug.cgi?id=1105939 --- src/security/security_selinux.c | 98 ++++++++++++----------------------------- 1 file changed, 29 insertions(+), 69 deletions(-)
Shouldn't there also be a change somewhere under tests/ to ensure that we parse XML in that situation?
I can add a test for parsing: <seclabel type='none' model='dac'/> but the only thing in seclabel XML parsing that depends on the security drivers is the default model that's filled in for <seclabel type='none'/> I'll send it separately as it wouldn't be directly related to the bug.
Otherwise, this looks like a fairly mechanical conversion of virDomainDefGetSecurityLabelDef() returning NULL from being a silent error into being successful; and that makes sense since domain_conf.c does not report any errors if a label cannot be looked up. ACK.
Thanks, pushed. Jan
participants (2)
-
Eric Blake -
Ján Tomko