From 6c4de1161425a610797495549349d194b90fb023 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Thu, 21 Mar 2013 16:12:55 +0100 Subject: [PATCH] security_manager: Don't manipulate domain XML in virDomainDefGetSecurityLabelDef 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 2b2f35c50a..f3fca7f3a5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1001,7 +1001,7 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def) return; } -static void +void virSecurityLabelDefFree(virSecurityLabelDefPtr def) { if (!def) @@ -1014,7 +1014,7 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr def) } -static void +void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def) { if (!def) @@ -16626,10 +16626,6 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) return def->seclabels[i]; } - seclabel = virDomainDefAddSecurityLabelDef(def, model); - if (seclabel) - seclabel->implicit = true; - return seclabel; } @@ -16664,55 +16660,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 c3b26083c5..edddf2545d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2298,10 +2298,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 a2c4a54c91..5812123540 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 c621366439..5c2a95b699 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 1e0063758a..60596ad757 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;