The virDomainDefGetSecurityLabelDef was modifying the domain XML. It tried to find a seclabel corresponding to given sec driver. If the label wasn't found, the function created one which is wrong. In fact it's security manager which should modify this part of domain XML. --- src/conf/domain_conf.c | 56 +++++++++++------------------------------ src/conf/domain_conf.h | 7 ++++-- src/libvirt_private.syms | 1 - src/security/security_manager.c | 40 ++++++++++++++++++++--------- src/security/security_selinux.c | 8 ++++-- 5 files changed, 53 insertions(+), 59 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b06cae5..8f5ae53 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1000,7 +1000,7 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def) return; } -static void +void virSecurityLabelDefFree(virSecurityLabelDefPtr def) { if (!def) @@ -1013,7 +1013,7 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr def) } -static void +void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def) { if (!def) @@ -16616,10 +16616,6 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) return def->seclabels[i]; } - seclabel = virDomainDefAddSecurityLabelDef(def, model); - if (seclabel) - seclabel->implicit = true; - return seclabel; } @@ -16654,55 +16650,31 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model) } virSecurityLabelDefPtr -virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) +virDomainDefGenSecurityLabelDef(const char *model) { virSecurityLabelDefPtr seclabel = NULL; - if (VIR_ALLOC(seclabel) < 0) - goto no_memory; - - if (model) { - seclabel->model = strdup(model); - if (seclabel->model == NULL) - goto no_memory; + if (VIR_ALLOC(seclabel) < 0 || + (model && !(seclabel->model = strdup(model)))) { + virReportOOMError(); + virSecurityLabelDefFree(seclabel); + seclabel = NULL; } - if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) - goto no_memory; - - def->seclabels[def->nseclabels - 1] = seclabel; - return seclabel; - -no_memory: - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; } virSecurityDeviceLabelDefPtr -virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +virDomainDiskDefGenSecurityLabelDef(const char *model) { virSecurityDeviceLabelDefPtr seclabel = NULL; - if (VIR_ALLOC(seclabel) < 0) - goto no_memory; - - if (model) { - seclabel->model = strdup(model); - if (seclabel->model == NULL) - goto no_memory; + if (VIR_ALLOC(seclabel) < 0 || + (model && !(seclabel->model = strdup(model)))) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + seclabel = NULL; } - if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) - goto no_memory; - - def->seclabels[def->nseclabels - 1] = seclabel; - return seclabel; - -no_memory: - virReportOOMError(); - virSecurityDeviceLabelDefFree(seclabel); - return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a750a1f..2540bca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2297,10 +2297,13 @@ virSecurityDeviceLabelDefPtr virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr -virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); +virDomainDefGenSecurityLabelDef(const char *model); virSecurityDeviceLabelDefPtr -virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); +virDomainDiskDefGenSecurityLabelDef(const char *model); + +void virSecurityLabelDefFree(virSecurityLabelDefPtr def); +void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def); typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 21bc615..e8085a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,7 +108,6 @@ virDomainControllerTypeToString; virDomainCpuPlacementModeTypeFromString; virDomainCpuPlacementModeTypeToString; virDomainDefAddImplicitControllers; -virDomainDefAddSecurityLabelDef; virDomainDefCheckABIStability; virDomainDefClearCCWAddresses; virDomainDefClearDeviceAliases; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c621366..5c2a95b 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -424,24 +424,26 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) { - int rc = 0; + int ret = -1; size_t i; virSecurityManagerPtr* sec_managers = NULL; virSecurityLabelDefPtr seclabel; + bool generated = false; if (mgr == NULL || mgr->drv == NULL) - return -1; + return ret; if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) - return -1; + return ret; virObjectLock(mgr); for (i = 0; sec_managers[i]; i++) { - seclabel = virDomainDefGetSecurityLabelDef(vm, - sec_managers[i]->drv->name); - if (seclabel == NULL) { - rc = -1; - goto cleanup; + generated = false; + seclabel = virDomainDefGetSecurityLabelDef(vm, sec_managers[i]->drv->name); + if (!seclabel) { + if (!(seclabel = virDomainDefGenSecurityLabelDef(sec_managers[i]->drv->name))) + goto cleanup; + generated = seclabel->implicit = true; } if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) { @@ -457,23 +459,37 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, sec_managers[i]->requireConfined) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unconfined guests are not allowed on this host")); - rc = -1; goto cleanup; } if (!sec_managers[i]->drv->domainGenSecurityLabel) { virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); } else { - rc += sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm); - if (rc) + /* The seclabel must be added to @vm prior calling domainGenSecurityLabel + * which may require seclabel to be presented already */ + + if (VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) < 0) { + if (VIR_DELETE_ELEMENT(vm->seclabels, + vm->nseclabels -1, vm->nseclabels) < 0) + vm->nseclabels--; goto cleanup; + } } } + ret = 0; + cleanup: virObjectUnlock(mgr); + if (generated) + virSecurityLabelDefFree(seclabel); VIR_FREE(sec_managers); - return rc; + return ret; } int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 1e00637..60596ad 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1161,11 +1161,15 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - disk_seclabel = - virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + disk_seclabel = virDomainDiskDefGenSecurityLabelDef(SECURITY_SELINUX_NAME); if (!disk_seclabel) return -1; disk_seclabel->norelabel = true; + if (VIR_APPEND_ELEMENT(disk->seclabels, disk->nseclabels, disk_seclabel) < 0) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(disk_seclabel); + return -1; + } ret = 0; } return ret; -- 1.8.1.5

On 28.03.2013 10:15, Daniel P. Berrange wrote: Woops, I've already pushed prior seeing your reply. However, There can be only 1 deletion, the virDomainDiskDefGenSecurityLabelDef() wasn't exported in libvirt_private.syms. I am pushing this follow up patch: commit a919e6f7769b27168b9217fd2fd5143259f63173 Author: Michal Privoznik <mprivozn(a)redhat.com&gt; AuthorDate: Thu Mar 28 10:39:25 2013 +0100 Commit: Michal Privoznik <mprivozn(a)redhat.com&gt; CommitDate: Thu Mar 28 10:39:25 2013 +0100 libvirt_private.syms: Correctly export seclabel APIs One of my previous patches manipulated virSecurityLabel* APIs, some were added to header files, and some were renamed. However, these changes were not reflected in libvirt_private.syms. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5812123..96eea0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -117,6 +117,7 @@ virDomainDefCopy; virDomainDefFormat; virDomainDefFormatInternal; virDomainDefFree; +virDomainDefGenSecurityLabelDef; virDomainDefGetSecurityLabelDef; virDomainDefParseFile; virDomainDefParseNode; @@ -138,6 +139,7 @@ virDomainDiskCopyOnReadTypeToString; virDomainDiskDefAssignAddress; virDomainDiskDefForeachPath; virDomainDiskDefFree; +virDomainDiskDefGenSecurityLabelDef; virDomainDiskDefGetSecurityLabelDef; virDomainDiskDeviceTypeToString; virDomainDiskErrorPolicyTypeFromString; @@ -341,6 +343,8 @@ virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; virDomainXMLConfGetNamespace; virDomainXMLConfNew; +virSecurityDeviceLabelDefFree; +virSecurityLabelDefFree; # conf/domain_event.h

https://bugzilla.redhat.com/show_bug.cgi?id=923946 The <seclabel type='none'/> should be added iff there is no other seclabel defined within a domain. This bug can be easily reproduced: 1) configure selinux seclabel for a domain 2) disable system's selinux and restart libvirtd 3) observe <seclabel type='none'/> being appended to a domain on its startup --- src/security/security_manager.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5c2a95b..b55af69 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -455,11 +455,16 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, } } - if ((seclabel->type == VIR_DOMAIN_SECLABEL_NONE) && - sec_managers[i]->requireConfined) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Unconfined guests are not allowed on this host")); - goto cleanup; + if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) { + if (sec_managers[i]->requireConfined) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unconfined guests are not allowed on this host")); + goto cleanup; + } else if (vm->nseclabels && generated) { + VIR_DEBUG("Skipping auto generated seclabel of type none"); + virSecurityLabelDefFree(seclabel); + continue; + } } if (!sec_managers[i]->drv->domainGenSecurityLabel) { -- 1.8.1.5