[PATCH v2 0/3] security: Don't error out on seclabels of type='none'
v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/OWDBS... diff to v1: - In patch 3/3 do not validate secmodel against known models. Michal Prívozník (3): conf: Fix seclabel type parsing wrt default value security: Rewrite virSecurityManagerCheckModel() to use g_autofree security: Don't error out on seclabels of type='none' src/conf/domain_conf.c | 13 ++++++------- src/security/security_manager.c | 33 ++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 18 deletions(-) -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Prior to v7.10.0-rc1~26 seclabels defaulted to VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching the parser to virXMLPropEnum() the type is overwritten to VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the helper function does is to set variable that holds the result to zero. Switch to virXMLPropEnumDefault() to restore the previous behavior. Fixes: f7ff8556ad9ba8d81408e31649071941a6a849a3 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Tested-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 84425ff39d..b1ee519ff6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7076,14 +7076,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (!(seclabel = virSecurityLabelDefNew(model))) return NULL; - /* set default value */ - seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; - - if (virXMLPropEnum(ctxt->node, "type", - virDomainSeclabelTypeFromString, - VIR_XML_PROP_NONZERO, - &seclabel->type) < 0) + if (virXMLPropEnumDefault(ctxt->node, "type", + virDomainSeclabelTypeFromString, + VIR_XML_PROP_NONZERO, + &seclabel->type, + VIR_DOMAIN_SECLABEL_DYNAMIC) < 0) { return NULL; + } if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || seclabel->type == VIR_DOMAIN_SECLABEL_NONE) -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Let's use automatic memory freeing inside of virSecurityManagerCheckModel() as it will simplify future commits. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Tested-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_manager.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5fc4eb4872..f2f3bb4f19 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -729,9 +729,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr, static int virSecurityManagerCheckModel(virSecurityManager *mgr, char *secmodel) { - int ret = -1; + g_autofree virSecurityManager **sec_managers = NULL; size_t i; - virSecurityManager **sec_managers = NULL; if (STREQ_NULLABLE(secmodel, "none")) return 0; @@ -741,17 +740,14 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, for (i = 0; sec_managers[i]; i++) { if (STREQ_NULLABLE(secmodel, sec_managers[i]->drv->name)) { - ret = 0; - goto cleanup; + return 0; } } virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Security driver model '%1$s' is not available"), secmodel); - cleanup: - VIR_FREE(sec_managers); - return ret; + return -1; } -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Ever since of commit v1.2.13-rc1~66 the model attribute of a <seclabel/> is validated against secdriver names enabled. In nearly all cases this is something users want so that domain XML does not claim to set seclabels of a model that's not enabled. However, consider the following seclabel: <seclabel type='none' model='selinux'/> It tells us to not bother setting selinux labels on given domain. A mgmt app might format this into domain XML if it sees selinux is disabled on the host. But if that's the case, selinux driver is not loaded and this virSecurityManagerCheckModel() doesn't find it and reports an error. Well, the error doesn't need to be reported as we will just ignore selinux as each driver callback checks if relabel is false (which it is for type='none'). This is true for other secdrivers too. Resolves: https://redhat.atlassian.net/browse/RHEL-156689 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f2f3bb4f19..bef9863799 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -727,7 +727,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr, static int virSecurityManagerCheckModel(virSecurityManager *mgr, - char *secmodel) + char *secmodel, + bool relabel) { g_autofree virSecurityManager **sec_managers = NULL; size_t i; @@ -744,6 +745,11 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, } } + if (relabel == false) { + VIR_INFO("Ignoring seclabel with model %s and relabel=no", secmodel); + return 0; + } + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Security driver model '%1$s' is not available"), secmodel); @@ -758,8 +764,11 @@ virSecurityManagerCheckDomainLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < def->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, def->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + def->seclabels[i]->model, + def->seclabels[i]->relabel) < 0) { return -1; + } } return 0; @@ -773,8 +782,11 @@ virSecurityManagerCheckDiskLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < disk->src->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, disk->src->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + disk->src->seclabels[i]->model, + disk->src->seclabels[i]->relabel) < 0) { return -1; + } } return 0; @@ -788,8 +800,11 @@ virSecurityManagerCheckChardevLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < dev->source->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, dev->source->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + dev->source->seclabels[i]->model, + dev->source->seclabels[i]->relabel) < 0) { return -1; + } } return 0; -- 2.52.0
I tested it again, and it still works! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Wed, Apr 01, 2026 at 13:40:52 +0200, Michal Privoznik via Devel wrote:
v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/OWDBS...
diff to v1: - In patch 3/3 do not validate secmodel against known models.
Michal Prívozník (3): conf: Fix seclabel type parsing wrt default value security: Rewrite virSecurityManagerCheckModel() to use g_autofree security: Don't error out on seclabels of type='none'
src/conf/domain_conf.c | 13 ++++++------- src/security/security_manager.c | 33 ++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 18 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Peter Krempa -
Richard W.M. Jones