[libvirt] [PATCH 2/4] security: add security part for shmem device
Luyao Huang
lhuang at redhat.com
Sun Aug 2 11:27:30 UTC 2015
Hi Marc-André
On 07/27/2015 11:39 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang<lhuang at redhat.com> wrote:
>> A new api to help set/restore the shmem deivce dac/selinux label.
> typo: deivce / device.
>
thanks, i will fix this in next version
>> Signed-off-by: Luyao Huang<lhuang at 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)
I forgot remove this, thanks for pointing out. (and i won't remove this,
since i need a function in virshm.h, so i will move it this patch after
patch 3/4 in next version)
>> #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
Okay
>> + 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.
>
Indeed, i will move the shmem_seclabel and seclabel check more early.
>> + 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
Okay
>> #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?
Hmm...these $path are the shm path, if the shmem device is server
enabled, the used shm is created by ivshmem-server, which will depends
on ivshmem-server's label. And qemu process will connect to the socket
created by ivshmem-server, so change the label for shm created by
ivshmem-sever is useless (also we don't know the shm name created by
ivshmem-server).
However, never mind, i will do a refactor and remove the variable $path
here to make this function easy to put in
virSecuritySELinuxSetSecurityAllLabel and
virSecuritySELinuxSetSecurityAllLabel.
>> + 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)
>
Good catch, i didn't notice that, i will add it in next version.
Luyao
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list