[libvirt] [PATCH v4 2/5] Multiple security drivers in XML data

Mon Aug 20 17:10:28 UTC 2012

On 16.08.2012 00:10, Marcelo Cerri wrote:
> This patch updates the domain and capability XML parser and formatter to
> support more than one "seclabel" element for each domain and device. The
> RNG schema and the tests related to this are also updated by this patch.
> 
> Signed-off-by: Marcelo Cerri <mhcerri at linux.vnet.ibm.com>
> ---
> docs/formatdomain.html.in | 11 +-
> docs/schemas/capability.rng | 18 +-
> docs/schemas/domaincommon.rng | 30 ++-
> src/conf/capabilities.c | 6 +-
> src/conf/domain_conf.c | 353 ++++++++++++++------
> src/conf/domain_conf.h | 9 +
> src/libvirt_private.syms | 3 +
> .../qemuxml2argv-seclabel-dynamic-override.xml | 4 +-
> .../qemuxml2argv-seclabel-dynamic.xml | 2 +-
> 9 files changed, 304 insertions(+), 132 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fb25cb8..85d2515 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1275,8 +1275,8 @@
> path to the file holding the disk. If the disk
> <code>type</code> is "block", then the <code>dev</code>
> attribute specifies the path to the host device to serve as
> - the disk. With both "file" and "block", an optional
> - sub-element <code>seclabel</code>, <a href="#seclabel">described
> + the disk. With both "file" and "block", one or more optional
> + sub-elements <code>seclabel</code>, <a href="#seclabel">described
> below</a> (and since 0.9.9), can be
> used to override the domain security labeling policy for just
> that source file. If the disk <code>type</code> is "dir", then the
> @@ -3880,6 +3880,13 @@ qemu-kvm -net nic,model=? /dev/null
> 
> 
> 
> + If more than one security driver is used by libvirt, multiple
> + <code>seclabel</code> tags can be used, one for each driver and
> + the security driver referenced by each tag can be defined using
> + the attribute <code>model</code>
> + 
> +
> + 
> Valid input XML configurations for the top-level security label
> are:
> 
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index c392e44..8c928bc 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -44,20 +44,22 @@
> <optional>
> <ref name='topology'/>
> </optional>
> - <optional>
> + <zeroOrMore>
> <ref name='secmodel'/>
> - </optional>
> + </zeroOrMore>
> </element>
> </define>
> 
> <define name='secmodel'>
> <element name='secmodel'>
> - <element name='model'>
> - <text/>
> - </element>
> - <element name='doi'>
> - <text/>
> - </element>
> + <interleave>
> + <element name='model'>
> + <text/>
> + </element>
> + <element name='doi'>
> + <text/>
> + </element>
> + </interleave>
> </element>
> </define>
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 1b94155..dd93f4d 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -55,9 +55,9 @@
> <optional>
> <ref name="devices"/>
> </optional>
> - <optional>
> + <zeroOrMore>
> <ref name="seclabel"/>
> - </optional>
> + </zeroOrMore>
> <optional>
> <ref name='qemucmdline'/>
> </optional>
> @@ -148,18 +148,32 @@
> 
> <choice>
> - <attribute name='relabel'>
> - <value>no</value>
> - </attribute>
> <group>
> <optional>
> + <attribute name='model'>
> + <text/>
> + </attribute>
> + </optional>
> + <attribute name='relabel'>
> + <value>no</value>
> + </attribute>
> + </group>
> + <group>
> + <optional>
> + <attribute name='model'>
> + <text/>
> + </attribute>
> + </optional>
> + <optional>
> <attribute name='relabel'>
> <value>yes</value>
> </attribute>
> </optional>
> - <element name='label'>
> - <text/>
> - </element>
> + <zeroOrMore>
> + <element name='label'>
> + <text/>
> + </element>
> + </zeroOrMore>
> </group>
> </choice>
> </element>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 9a0bc3d..bc6266e 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -772,12 +772,12 @@ virCapabilitiesFormatXML(virCapsPtr caps)
> virBufferAddLit(&xml, " </topology>\n");
> }
> 
> - if (caps->host.nsecModels) {
> + for (i = 0; i < caps->host.nsecModels; i++) {
> virBufferAddLit(&xml, " <secmodel>\n");
> virBufferAsprintf(&xml, " <model>%s</model>\n",
> - caps->host.secModels[0].model);
> + caps->host.secModels[i].model);
> virBufferAsprintf(&xml, " <doi>%s</doi>\n",
> - caps->host.secModels[0].doi);
> + caps->host.secModels[i].doi);
> virBufferAddLit(&xml, " </secmodel>\n");
> }
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 98daf32..2dea33e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3067,17 +3067,19 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
> return 0;
> }
> 
> -static int
> -virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> - xmlXPathContextPtr ctxt,
> +static virSecurityLabelDefPtr
> +virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
> unsigned int flags)
> {
> char *p;
> + virSecurityLabelDefPtr def = NULL;
> 
> - if (virXPathNode("./seclabel[1]", ctxt) == NULL)
> - return 0;
> + if (VIR_ALLOC(def) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> 
> - p = virXPathStringLimit("string(./seclabel[1]/@type)",
> + p = virXPathStringLimit("string(./@type)",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> if (p == NULL) {
> def->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
> @@ -3091,7 +3093,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> }
> }
> 
> - p = virXPathStringLimit("string(./seclabel[1]/@relabel)",
> + p = virXPathStringLimit("string(./@relabel)",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> if (p != NULL) {
> if (STREQ(p, "yes")) {
> @@ -3131,7 +3133,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
> (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> - p = virXPathStringLimit("string(./seclabel[1]/label[1])",
> + p = virXPathStringLimit("string(./label[1])",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> if (p == NULL) {
> virReportError(VIR_ERR_XML_ERROR,
> @@ -3146,7 +3148,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> if (!def->norelabel &&
> (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> - p = virXPathStringLimit("string(./seclabel[1]/imagelabel[1])",
> + p = virXPathStringLimit("string(./imagelabel[1])",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> if (p == NULL) {
> virReportError(VIR_ERR_XML_ERROR,
> @@ -3158,93 +3160,179 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> 
> /* Only parse baselabel for dynamic label type */
> if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> - p = virXPathStringLimit("string(./seclabel[1]/baselabel[1])",
> + p = virXPathStringLimit("string(./baselabel[1])",
> VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> def->baselabel = p;
> }
> 
> - /* Only parse model, if static labelling, or a base
> - * label is set, or doing active XML
> - */
> - if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
> - def->baselabel ||
> - (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> - def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> - p = virXPathStringLimit("string(./seclabel[1]/@model)",
> - VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> - if (p == NULL) {
> - virReportError(VIR_ERR_XML_ERROR,
> - "%s", _("missing security model"));
> - goto error;
> - }
> - def->model = p;
> + /* Always parse model */
> + p = virXPathStringLimit("string(./@model)",
> + VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> + if (p == NULL && def->type != VIR_DOMAIN_SECLABEL_NONE) {
> + virReportError(VIR_ERR_XML_ERROR,
> + "%s", _("missing security model"));
> }
> + def->model = p;
> 
> - return 0;
> + return def;
> 
> error:
> virSecurityLabelDefFree(def);
> + return NULL;
> +}
> +
> +static int
> +virSecurityLabelDefsParseXML(virDomainDefPtr def,
> + xmlXPathContextPtr ctxt,
> + unsigned int flags)
> +{
> + int i, n;

'i' needs to be initialized, otherwise ..

> + xmlNodePtr *list = NULL, saved_node;
> +
> + /* Check args and save context */
> + if (def == NULL || ctxt == NULL)
> + return 0;
> + saved_node = ctxt->node;
> +
> + /* Allocate a security labels based on XML */
> + if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0)
> + return 0;
> +
> + if (VIR_ALLOC_N(def->seclabels, n) < 0) {
> + virReportOOMError();
> + goto error;

.. we might jump to error here ..

> + }
> +
> + /* Parse each "seclabel" tag */
> + for (i = 0; i < n; i++) {
> + ctxt->node = list[i];
> + def->seclabels[i] = virSecurityLabelDefParseXML(ctxt, flags);
> + if (def->seclabels[i] == NULL)
> + goto error;
> + }
> + def->nseclabels = n;
> + ctxt->node = saved_node;
> + VIR_FREE(list);
> +
> + /* Checking missing model information
> + * when there is more than one seclabel */
> + if (n > 1) {
> + for(; n; n--) {
> + if (def->seclabels[n - 1]->model == NULL) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing security model "
> + "when using multiple labels"));
> + goto error;
> + }
> + }
> + }
> + return 0;
> +
> +error:
> + ctxt->node = saved_node;
> + for (; i > 0; i--) {

and use 'i' uninitialized.

side note: It took me quite some time to identify this as I had hit gcc
bug here. For some reason it was reporting uninitialized variable 'i'
but in totally different function virDomainDefParseXML. I guess since
virSecurityLabelDefsParseXML is static, gcc might have inlined it and
hence the wrong function name.

> + virSecurityLabelDefFree(def->seclabels[i - 1]);
> + }
> + VIR_FREE(def->seclabels);
> + VIR_FREE(list);
> return -1;
> }
> 

ACK with this squashed in:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8f9f6de..c9f5a3c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3104,7 +3104,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
                             xmlXPathContextPtr ctxt,
                             unsigned int flags)
 {
-    int i, n;
+    int i = 0, n;
     xmlNodePtr *list = NULL, saved_node;

     /* Check args and save context */



