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

Re: [libvirt] [PATCH] conf: Fix parsing of seclabels without model



On Thu, Aug 30, 2012 at 13:19:31 -0300, Marcelo Cerri wrote:
> With this patch libvirt tries to assign a model to seclabels when model
> is missing. Libvirt will look up at host's capabilities and assign a
> model in order to each seclabel that doesn't have a model assigned.
> 
> This patch fixes:
> 
> 1. The problem with existing guests that have a seclabel defined in its XML.
> 2. A XML parse error when a guest is restored.
> 
> Signed-off-by: Marcelo Cerri <mhcerri linux vnet ibm com>
> ---
>  src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 224aec5..5316b59 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3102,22 +3102,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr 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(./@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;
> -    }
> +    /* Always parse model */
> +    p = virXPathStringLimit("string(./@model)",
> +                            VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> +    def->model = p;
>  
>      return def;
>  

This hunk looks correct.

> @@ -3129,10 +3117,12 @@ error:
>  static int
>  virSecurityLabelDefsParseXML(virDomainDefPtr def,
>                              xmlXPathContextPtr ctxt,
> +                            virCapsPtr caps,
>                              unsigned int flags)
>  {
> -    int i = 0, n;
> +    int i, j, n;
>      xmlNodePtr *list = NULL, saved_node;
> +    virCapsHostPtr host = &caps->host;
>  
>      /* Check args and save context */
>      if (def == NULL || ctxt == NULL)
> @@ -3159,14 +3149,26 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
>      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) {
> +    /* Check missing model information */
> +    for (i = j = 0; i < n; i++) {
> +        /* If model is missing, try to assign it based on driver's
> +         * capabilities.
> +         */
> +        if (def->seclabels[i]->model == NULL) {
> +            /* Check if there's any host's security model that wasn't
> +             * assigned yet.
> +             */
> +            if (j >= host->nsecModels) {
>                  virReportError(VIR_ERR_XML_ERROR, "%s",
> -                                     _("missing security model "
> -                                       "when using multiple labels"));
> +                                     _("missing security model and "
> +                                       "it can't be assigned based on "
> +                                       "host's capabilities"));
> +                goto error;
> +            }
> +            /* Copy model from host. */
> +            def->seclabels[i]->model = strdup(host->secModels[j++].model);
> +            if (def->seclabels[i]->model == NULL) {
> +                virReportOOMError();
>                  goto error;
>              }
>          }

But this seems wrong. The only case when model can be missing is when there
is just one seclabel defined and either type is none or type is dynamic,
baselabel is not defined and INACTIVE flags is set. This is the only case in
which we need to guess what model was used and we should be able to just use
the first secModel for that. That is the code is not incorrect but relaxes
the requirements too much. We should require model to be present in all
cases except for the one case needed for backward compatibility.

Jirka


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