We do have a check for valid per-domain security model, however we still do permit an invalid security model for a <disk> type device. This patch introduces a new function virSecurityStackCheckDiskLabels which compares user specified security model against currently registered security drivers. That being said, it also permits 'none' being specified as a device security model. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/security/security_manager.c | 5 +++++ src/security/security_manager.h | 1 + src/security/security_stack.c | 48 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 302f54d..8eacf0c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -262,6 +262,11 @@ virSecurityManagerGetDriver(virSecurityManagerPtr mgr) return mgr->virtDriver; } +const char * +virSecurityManagerGetDriverName(virSecurityManagerPtr mgr) +{ + return mgr->drv->name; +} const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 156f882..4626c4b 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -70,6 +70,7 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriverName(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 1ded57b..75d8b96 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -301,7 +301,46 @@ virSecurityStackRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackCheckSecurityDiskLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + size_t i, j; + bool sec_model_valid = false; + + for (i = 0; i < vm->ndisks; i++) { + virStorageSourcePtr src = vm->disks[i]->src; + for (j = 0; j < src->nseclabels; j++) { + const char *sec_model = src->seclabels[j]->model; + + if (STREQ_NULLABLE(sec_model, "none")) { + sec_model_valid = true; + continue; + } + + sec_model_valid = false; + for (; item; item = item->next) { + const char *drv_name = virSecurityManagerGetDriverName(mgr); + + if (STREQ_NULLABLE(sec_model, drv_name)) { + sec_model_valid = true; + break; + } + } + if (!sec_model_valid) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No security driver matches security model " + "'%s'"), + sec_model); + return -1; + } + } + } + return 0; +} static int virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -309,13 +348,18 @@ virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; + int rc = -1; + + if (virSecurityStackCheckSecurityDiskLabels(mgr, vm) < 0) + goto error; for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path) < 0) - rc = -1; + goto error; } + rc = 0; + error: return rc; } -- 1.9.3

--- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4251b13..a36dace 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5213,7 +5213,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, 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 for model %s is already provided"), seclabel->model); virSecurityLabelDefFree(seclabel); goto error; -- 1.9.3

Parser checks for per-domain seclabel duplicates, so it would be nice if it checked for per-device seclabel duplicates the same way Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/conf/domain_conf.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a36dace..c43aded 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5323,6 +5323,15 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, break; } } + + /* check for duplicate seclabels */ + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(model, seclabels[j]->model)) { + virReportError(VIR_ERR_XML_DETAIL, + _("seclabel for model %s is already provided"), model); + goto error; + } + } seclabels[i]->model = model; } -- 1.9.3

This is only tested by domainschematest. It should be added to either qemuxml2xmltest or qemuxml2argvtest and squashed in together with the commit adding this check. Jan On Fri, Feb 06, 2015 at 07:13:27PM +0100, Erik Skultety wrote: