Re: [PATCH 2/4] security: add security part for shmem device

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

 



Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang@xxxxxxxxxx> wrote:
> A new api to help set/restore the shmem deivce dac/selinux label.

typo: deivce / device.

>
> Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> ---
>  src/libvirt_private.syms        |  2 ++
>  src/security/security_dac.c     | 67 +++++++++++++++++++++++++++++++++++++++
>  src/security/security_driver.h  | 11 +++++++
>  src/security/security_manager.c | 38 ++++++++++++++++++++++
>  src/security/security_manager.h |  8 +++++
>  src/security/security_selinux.c | 70 +++++++++++++++++++++++++++++++++++++++++
>  src/security/security_stack.c   | 41 ++++++++++++++++++++++++
>  7 files changed, 237 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 588b1c4..af73177 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1038,6 +1038,7 @@ virSecurityManagerRestoreDiskLabel;
>  virSecurityManagerRestoreHostdevLabel;
>  virSecurityManagerRestoreImageLabel;
>  virSecurityManagerRestoreSavedStateLabel;
> +virSecurityManagerRestoreShmemLabel;
>  virSecurityManagerSetAllLabel;
>  virSecurityManagerSetChildProcessLabel;
>  virSecurityManagerSetDaemonSocketLabel;
> @@ -1048,6 +1049,7 @@ virSecurityManagerSetImageFDLabel;
>  virSecurityManagerSetImageLabel;
>  virSecurityManagerSetProcessLabel;
>  virSecurityManagerSetSavedStateLabel;
> +virSecurityManagerSetShmemLabel;
>  virSecurityManagerSetSocketLabel;
>  virSecurityManagerSetTapFDLabel;
>  virSecurityManagerStackAddNested;
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index deb6980..f954aa5 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -39,6 +39,7 @@
>  #include "virstoragefile.h"
>  #include "virstring.h"
>  #include "virutil.h"
> +#include "virshm.h"

This header doesn't exist (yet)

>
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> @@ -922,6 +923,69 @@ virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr,
>
>
>  static int
> +virSecurityDACSetShmemLabel(virSecurityManagerPtr mgr,
> +                            virDomainDefPtr def,
> +                            virDomainShmemDefPtr shmem,
> +                            char *path)
> +{
> +    virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityLabelDefPtr seclabel;
> +    virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +    char *tmppath;

could make it const

> +    uid_t user;
> +    gid_t group;
> +
> +    if (shmem->server.enabled)
> +        tmppath = shmem->server.chr.data.nix.path;
> +    else
> +        tmppath = path;
> +
> +    if (!tmppath)
> +        return 0;
> +
> +    shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_DAC_NAME);
> +
> +    if (shmem_seclabel && !shmem_seclabel->relabel)
> +        return 0;
> +
> +    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> +

The function is similar to virSecurityDACSetSecurityImageLabel and yet
subtly different: there is a early dynamicOwnership condition that
seems to be general, the domain seclabel->relabel is checked first. It
would be nice to align the behaviour.


> +    if (shmem_seclabel && shmem_seclabel->label) {
> +        if (virParseOwnershipIds(shmem_seclabel->label, &user, &group) < 0)
> +            return -1;
> +    } else {
> +        if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0)
> +            return -1;
> +    }
> +
> +    return virSecurityDACSetOwnership(tmppath, user, group);
> +}
> +
> +
> +static int
> +virSecurityDACRestoreShmemLabel(virSecurityManagerPtr mgr,
> +                               virDomainDefPtr def,
> +                               virDomainShmemDefPtr shmem,
> +                               char *path)
> +{
> +    virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +
> +    shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_DAC_NAME);
> +
> +    if (shmem_seclabel && !shmem_seclabel->relabel)
> +        return 0;
> +
> +    if (shmem->server.enabled)
> +        return virSecurityDACRestoreChardevLabel(mgr, def, NULL, &shmem->server.chr);
> +
> +    if (!path)
> +        return 0;
> +
> +    return virSecurityDACRestoreSecurityFileLabel(path);
> +}
> +
> +
> +static int
>  virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr,
>                                        virDomainDefPtr def,
>                                        bool migrated)
> @@ -1433,4 +1497,7 @@ virSecurityDriver virSecurityDriverDAC = {
>      .domainGetSecurityMountOptions      = virSecurityDACGetMountOptions,
>
>      .getBaseLabel                       = virSecurityDACGetBaseLabel,
> +
> +    .domainSetSecurityShmemLabel        = virSecurityDACSetShmemLabel,
> +    .domainRestoreSecurityShmemLabel    = virSecurityDACRestoreShmemLabel,
>  };
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index f0dca09..37e4527 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -118,6 +118,14 @@ typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr,
>  typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr,
>                                                     virDomainDefPtr def,
>                                                     virStorageSourcePtr src);
> +typedef int (*virSecurityDomainSetShmemLabel) (virSecurityManagerPtr mgr,
> +                                               virDomainDefPtr def,
> +                                               virDomainShmemDefPtr shmem,
> +                                               char *path);
> +typedef int (*virSecurityDomainRestoreShmemLabel) (virSecurityManagerPtr mgr,
> +                                                   virDomainDefPtr def,
> +                                                   virDomainShmemDefPtr shmem,
> +                                                   char *path);
>
>
>  struct _virSecurityDriver {
> @@ -168,6 +176,9 @@ struct _virSecurityDriver {
>      virSecurityDomainSetHugepages domainSetSecurityHugepages;
>
>      virSecurityDriverGetBaseLabel getBaseLabel;
> +
> +    virSecurityDomainSetShmemLabel domainSetSecurityShmemLabel;
> +    virSecurityDomainRestoreShmemLabel domainRestoreSecurityShmemLabel;
>  };
>
>  virSecurityDriverPtr virSecurityDriverLookup(const char *name,
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index b0cd9e8..72ca7e2 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -991,3 +991,41 @@ virSecurityManagerSetHugepages(virSecurityManagerPtr mgr,
>
>      return 0;
>  }
> +
> +
> +int
> +virSecurityManagerRestoreShmemLabel(virSecurityManagerPtr mgr,
> +                                    virDomainDefPtr vm,
> +                                    virDomainShmemDefPtr shmem,
> +                                    char *path)
> +{
> +    if (mgr->drv->domainRestoreSecurityShmemLabel) {
> +        int ret;
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainRestoreSecurityShmemLabel(mgr, vm, shmem, path);
> +        virObjectUnlock(mgr);
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +    return -1;
> +}
> +
> +
> +int
> +virSecurityManagerSetShmemLabel(virSecurityManagerPtr mgr,
> +                                virDomainDefPtr vm,
> +                                virDomainShmemDefPtr shmem,
> +                                char *path)
> +{
> +    if (mgr->drv->domainSetSecurityShmemLabel) {
> +        int ret;
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainSetSecurityShmemLabel(mgr, vm, shmem, path);
> +        virObjectUnlock(mgr);
> +        return ret;
> +    }
> +
> +    virReportUnsupportedError();
> +    return -1;
> +}
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 13468db..ce37c91 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -149,5 +149,13 @@ int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,
>  int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr,
>                                          virDomainDefPtr vm,
>                                          virStorageSourcePtr src);
> +int virSecurityManagerRestoreShmemLabel(virSecurityManagerPtr mgr,
> +                                        virDomainDefPtr vm,
> +                                        virDomainShmemDefPtr shmem,
> +                                        char *path);
> +int virSecurityManagerSetShmemLabel(virSecurityManagerPtr mgr,
> +                                    virDomainDefPtr vm,
> +                                    virDomainShmemDefPtr shmem,
> +                                    char *path);

const path

>
>  #endif /* VIR_SECURITY_MANAGER_H__ */
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 6e67a86..cbf89ee 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -46,6 +46,7 @@
>  #include "virconf.h"
>  #include "virtpm.h"
>  #include "virstring.h"
> +#include "virshm.h"

remove that too

>
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> @@ -1888,6 +1889,37 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def,
>  }
>
>
> +static int
> +virSecuritySELinuxRestoreShmemLabel(virSecurityManagerPtr mgr,
> +                                    virDomainDefPtr def,
> +                                    virDomainShmemDefPtr shmem,
> +                                    char *path)

const path

> +{
> +    char *tmppath = NULL;

make it const too

> +    virSecurityLabelDefPtr seclabel;
> +    virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +
> +    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> +    if (!seclabel || !seclabel->relabel)
> +        return 0;
> +
> +    shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_SELINUX_NAME);
> +
> +    if (shmem_seclabel && !shmem_seclabel->relabel)
> +        return 0;
> +
> +    if (shmem->server.enabled)
> +        tmppath = shmem->server.chr.data.nix.path;
> +    else
> +        tmppath = path;
> +
> +    if (!tmppath)
> +        return 0;
> +
> +    return virSecuritySELinuxRestoreSecurityFileLabel(mgr, tmppath);
> +}
> +
> +
>  static const char *
>  virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int virtType)
>  {
> @@ -2284,6 +2316,41 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
>
>
>  static int
> +virSecuritySELinuxSetShmemLabel(virSecurityManagerPtr mgr,
> +                                virDomainDefPtr def,
> +                                virDomainShmemDefPtr shmem,
> +                                char *path)
> +{
> +    virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
> +    char *tmppath = NULL;
> +    virSecurityLabelDefPtr seclabel;
> +    virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +
> +    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> +    if (!seclabel || !seclabel->relabel)
> +        return 0;
> +
> +    shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_SELINUX_NAME);
> +
> +    if (shmem_seclabel && !shmem_seclabel->relabel)
> +        return 0;
> +
> +    if (shmem->server.enabled)
> +        tmppath = shmem->server.chr.data.nix.path;
> +    else
> +        tmppath = path;

I am not sure it's a good idea to either set the server socket policy
or the shm. Why not set both?

> +    if (!tmppath)
> +        return 0;
> +
> +    if (shmem_seclabel && shmem_seclabel->label)
> +        return virSecuritySELinuxSetFilecon(tmppath, shmem_seclabel->label);
> +    else
> +        return virSecuritySELinuxSetFilecon(tmppath, data->file_context);
> +}
> +
> +
> +static int
>  virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
>                                        virDomainDefPtr def,
>                                        const char *stdin_path)
> @@ -2549,4 +2616,7 @@ virSecurityDriver virSecurityDriverSELinux = {
>
>      .domainGetSecurityMountOptions      = virSecuritySELinuxGetSecurityMountOptions,
>      .getBaseLabel                       = virSecuritySELinuxGetBaseLabel,
> +
> +    .domainSetSecurityShmemLabel        = virSecuritySELinuxSetShmemLabel,
> +    .domainRestoreSecurityShmemLabel    = virSecuritySELinuxRestoreShmemLabel,
>  };
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index 1ded57b..22c1b56 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -599,6 +599,44 @@ virSecurityStackRestoreSecurityImageLabel(virSecurityManagerPtr mgr,
>      return rc;
>  }
>
> +static int
> +virSecurityStackSetShmemLabel(virSecurityManagerPtr mgr,
> +                              virDomainDefPtr vm,
> +                              virDomainShmemDefPtr shmem,
> +                              char *path)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerSetShmemLabel(item->securityManager,
> +                                            vm, shmem, path) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +static int
> +virSecurityStackRestoreShmemLabel(virSecurityManagerPtr mgr,
> +                                  virDomainDefPtr vm,
> +                                  virDomainShmemDefPtr shmem,
> +                                  char *path)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerRestoreShmemLabel(item->securityManager,
> +                                                vm, shmem, path) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
>  virSecurityDriver virSecurityDriverStack = {
>      .privateDataLen                     = sizeof(virSecurityStackData),
>      .name                               = "stack",
> @@ -648,4 +686,7 @@ virSecurityDriver virSecurityDriverStack = {
>      .domainSetSecurityHugepages         = virSecurityStackSetHugepages,
>
>      .getBaseLabel                       = virSecurityStackGetBaseLabel,
> +
> +    .domainSetSecurityShmemLabel        = virSecurityStackSetShmemLabel,
> +    .domainRestoreSecurityShmemLabel    = virSecurityStackRestoreShmemLabel,
>  };
> --
> 1.8.3.1

Shouldn't it be implemented for the nop virSecurityDriver too? (note:
I don't know what it is for)

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



-- 
Marc-André Lureau
7346 2483 9404 4E20 ABFF  7D48 D864 9487 F43F 0992

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]