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

Hi On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang<lhuang(a)redhat.com> wrote: > A new api to help set/restore the shmem deivce dac/selinux label. typo: deivce / device.

> Signed-off-by: Luyao Huang<lhuang(a)redhat.com> > --- > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list