[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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 <span class="since">since 0.9.9</span>), 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
>      </p>
>  
>      <p>
> +      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>
> +    </p>
> +
> +    <p>
>        Valid input XML configurations for the top-level security label
>        are:
>      </p>
> 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 @@
>        <!-- A per-device seclabel override is more limited, either
>             relabel=no or a <label> must be present.  -->
>        <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 */



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]