Re: [PATCH 07/10] selinux: Resolve resource leak using the default disk label

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On 01/17/13 20:17, John Ferlan wrote:
> Commit id a994ef2d1 changed the mechanism to store/update the default
> security label from using disk->seclabels[0] to allocating one on the
> fly. That change allocated the label, but never saved it.  This patch
> will save the label.
> ---
>   src/conf/domain_conf.c          | 29 +++++++++++++++++++++++++++++
>   src/conf/domain_conf.h          |  3 +++
>   src/security/security_selinux.c |  6 +++---
>   3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0b9ba13..0b3427f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16064,3 +16064,32 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model)
>
>       return seclabel;
>   }
> +
> +virSecurityDeviceLabelDefPtr
> +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model)
> +{
> +    virSecurityDeviceLabelDefPtr seclabel = NULL;
> +
> +    if (VIR_ALLOC(seclabel) < 0) {
> +        virReportOOMError();

all ...

> +        return NULL;
> +    }
> +
> +    if (model) {
> +        seclabel->model = strdup(model);
> +        if (seclabel->model == NULL) {
> +            virReportOOMError();
> +            virSecurityDeviceLabelDefFree(seclabel);

.. these ..

> +            return NULL;
> +        }
> +    }
> +
> +    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) {
> +        virReportOOMError();
> +        virSecurityDeviceLabelDefFree(seclabel);

... error paths can be merged under a no_memory label saving a few lines 
of code. (virSecurityDeviceLabelDefFree handles NULL arguments gracefully)

> +        return NULL;
> +    }
> +    def->seclabels[def->nseclabels - 1] = seclabel;
> +
> +    return seclabel;
> +}
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ce36003..9770ffb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
>   virSecurityLabelDefPtr
>   virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
>
> +virSecurityDeviceLabelDefPtr
> +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model);
> +
>   typedef const char* (*virEventActionToStringFunc)(int type);
>   typedef int (*virEventActionFromStringFunc)(const char *type);
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b5e1a9a..511e923 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk,
>       if (ret == 1 && !disk_seclabel) {
>           /* If we failed to set a label, but virt_use_nfs let us
>            * proceed anyway, then we don't need to relabel later.  */
> -        if (VIR_ALLOC(disk_seclabel) < 0) {
> -            virReportOOMError();
> +        disk_seclabel =
> +            virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME);
> +        if (!disk_seclabel)
>               return -1;
> -        }
>           disk_seclabel->norelabel = true;
>           ret = 0;
>       }

Otherwise this seems to me be a good approach how to fix it, but I'm not 
that familiar with the selinux code. Hopefully, if this fix is wrong 
somebody will chime in on the v2.

Peter

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list