In this patch we label the swtpm process with SELinux labels. We give it the
same label as the QEMU process has. We label its state directory and files
as well. We restore the old security labels once the swtpm has terminated.

The file and process labels now look as follows:

Directory: /var/lib/libvirt/swtpm

[root@localhost swtpm]# ls -lZ
total 4
rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm

[root@localhost testvm]# ls -lZ
total 8
-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall

The log in /var/log/swtpm/libvirt/qemu is labeled as follows:

-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 src/libvirt_private.syms        |   2 +
 src/qemu/qemu_tpm.c             |  24 +++++-
 src/security/security_driver.h  |   7 ++
 src/security/security_manager.c |  36 +++++++++
 src/security/security_manager.h |   6 ++
 src/security/security_selinux.c | 164 ++++++++++++++++++++++++++++++++++++++++
 src/security/security_stack.c   |  40 ++++++++++
 7 files changed, 278 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 75b8932..2ce67e7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
 virSecurityManagerRestoreInputLabel;
 virSecurityManagerRestoreMemoryLabel;
 virSecurityManagerRestoreSavedStateLabel;
+virSecurityManagerRestoreTPMLabels;
 virSecurityManagerSetAllLabel;
 virSecurityManagerSetChardevLabel;
 virSecurityManagerSetChildProcessLabel;
@@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
 virSecurityManagerSetSavedStateLabel;
 virSecurityManagerSetSocketLabel;
 virSecurityManagerSetTapFDLabel;
+virSecurityManagerSetTPMLabels;
 virSecurityManagerStackAddNested;
 virSecurityManagerTransactionAbort;
 virSecurityManagerTransactionCommit;
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 024d24d..62f0146 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
 
     virCommandSetErrorBuffer(cmd, &errbuf);
 
-    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
+    if (virSecurityManagerSetTPMLabels(driver->securityManager,
+                                       def) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
+                                               def, cmd) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerPreFork(driver->securityManager) < 0)
+        goto cleanup;
+
+    /* make sure we run this with the appropriate user */
+    virCommandSetUID(cmd, cfg->swtpm_user);
+    virCommandSetGID(cmd, cfg->swtpm_group);
+
+    ret = virCommandRun(cmd, &exitstatus);
+
+    virSecurityManagerPostFork(driver->securityManager);
+
+    if (ret < 0 || exitstatus != 0) {
         VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
                     "stderr: %s"), exitstatus, errbuf);
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -696,6 +715,8 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
     ret = 0;
 
  cleanup:
+    if (ret < 0)
+        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
     VIR_FREE(shortName);
     VIR_FREE(errbuf);
     virCommandFree(cmd);
@@ -741,6 +762,7 @@ qemuExtTPMStop(virQEMUDriverPtr driver,
             goto cleanup;
 
         qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
+        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
         break;
     case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
     case VIR_DOMAIN_TPM_TYPE_LAST:
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 95e7c4d..cbf0ecf 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -149,6 +149,10 @@ typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr,
                                                      virDomainDefPtr def,
                                                      virDomainChrSourceDefPtr dev_source,
                                                      bool chardevStdioLogd);
+typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManagerPtr mgr,
+                                              virDomainDefPtr def);
+typedef int (*virSecurityDomainRestoreTPMLabels) (virSecurityManagerPtr mgr,
+                                                  virDomainDefPtr def);
 
 
 struct _virSecurityDriver {
@@ -213,6 +217,9 @@ struct _virSecurityDriver {
 
     virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel;
     virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel;
+
+    virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels;
+    virSecurityDomainRestoreTPMLabels domainRestoreSecurityTPMLabels;
 };
 
 virSecurityDriverPtr virSecurityDriverLookup(const char *name,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 71f7f59..8683ad7 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1204,3 +1204,39 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
     virReportUnsupportedError();
     return -1;
 }
+
+
+int
+virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
+                               virDomainDefPtr vm)
+{
+    int ret;
+
+    if (mgr->drv->domainSetSecurityTPMLabels) {
+        virObjectLock(mgr);
+        ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm);
+        virObjectUnlock(mgr);
+
+        return ret;
+    }
+
+    return 0;
+}
+
+
+int
+virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
+                                   virDomainDefPtr vm)
+{
+    int ret;
+
+    if (mgr->drv->domainRestoreSecurityTPMLabels) {
+        virObjectLock(mgr);
+        ret = mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm);
+        virObjectUnlock(mgr);
+
+        return ret;
+    }
+
+    return 0;
+}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index c36a8b4..e772b61 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -194,4 +194,10 @@ int virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
                                           virDomainChrSourceDefPtr dev_source,
                                           bool chardevStdioLogd);
 
+int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
+                                   virDomainDefPtr vm);
+
+int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
+                                       virDomainDefPtr vm);
+
 #endif /* VIR_SECURITY_MANAGER_H__ */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 92e8415..6377fb7 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3048,6 +3048,167 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr,
     return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel);
 }
 
+
+/*
+ * _virSecuritySELinuxSetFileLabels:
+ *
+ * @mgr: the virSecurityManager
+ * @path: path to a directory or a file
+ * @seclabel: the security label
+ *
+ * Set the file labels on the given path; if the path is a directory
+ * we label all files found there, including the directory itself,
+ * otherwise we just label the file.
+ */
+static int
+_virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr,
+                                 const char *path,
+                                 virSecurityLabelDefPtr seclabel)
+{
+    int ret = 0;
+    struct dirent *ent;
+    char *filename = NULL;
+    DIR *dir;
+
+    if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel)))
+        return ret;
+
+    if (!virFileIsDir(path))
+        return 0;
+
+    if (virDirOpen(&dir, path) < 0)
+        return -1;
+
+    while ((ret = virDirRead(dir, &ent, path)) > 0) {
+        if (ent->d_type != DT_REG)
+            continue;
+
+        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
+            ret = -1;
+            break;
+        }
+        ret = virSecuritySELinuxSetFilecon(mgr, filename,
+                                           seclabel->imagelabel);
+        VIR_FREE(filename);
+        if (ret < 0)
+            break;
+    }
+    if (ret < 0)
+        virReportSystemError(errno, _("Unable to label files under %s"),
+                             path);
+
+    virDirClose(&dir);
+
+    return ret;
+}
+
+
+/*
+ * _virSecuritySELinuxRestoreFileLabels:
+ *
+ * @mgr: the virSecurityManager
+ * @path: path to a directory or a file
+ *
+ * Restore the file labels on the given path; if the path is a directory
+ * we restore all file labels found there, including the label of the
+ * directory itself, otherwise we just restore the label on the file.
+ */
+static int
+_virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr,
+                                     const char *path)
+{
+    int ret = 0;
+    struct dirent *ent;
+    char *filename = NULL;
+    DIR *dir;
+
+    if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path)))
+        return ret;
+
+    if (!virFileIsDir(path))
+        return 0;
+
+    if (virDirOpen(&dir, path) < 0)
+        return -1;
+
+    while ((ret = virDirRead(dir, &ent, path)) > 0) {
+        if (ent->d_type != DT_REG)
+            continue;
+
+        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
+            ret = -1;
+            break;
+        }
+        ret = virSecuritySELinuxRestoreFileLabel(mgr, filename);
+        VIR_FREE(filename);
+        if (ret < 0)
+            break;
+    }
+    if (ret < 0)
+        virReportSystemError(errno, _("Unable to restore file labels under %s"),
+                             path);
+
+    virDirClose(&dir);
+
+    return ret;
+}
+
+
+static int
+virSecuritySELinuxSetTPMLabels(virSecurityManagerPtr mgr,
+                               virDomainDefPtr def)
+{
+    int ret = 0;
+    virSecurityLabelDefPtr seclabel;
+
+    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
+    if (seclabel == NULL)
+        return 0;
+
+    switch (def->tpm->type) {
+    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+        break;
+    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
+        ret = _virSecuritySELinuxSetFileLabels(
+            mgr, def->tpm->data.emulator.storagepath,
+            seclabel);
+        if (ret == 0 && def->tpm->data.emulator.logfile)
+            ret = _virSecuritySELinuxSetFileLabels(
+                mgr, def->tpm->data.emulator.logfile,
+                seclabel);
+        break;
+    case VIR_DOMAIN_TPM_TYPE_LAST:
+        break;
+    }
+
+    return ret;
+}
+
+
+static int
+virSecuritySELinuxRestoreTPMLabels(virSecurityManagerPtr mgr,
+                                   virDomainDefPtr def)
+{
+    int ret = 0;
+
+    switch (def->tpm->type) {
+    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+        break;
+    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
+        ret = _virSecuritySELinuxRestoreFileLabels(
+            mgr, def->tpm->data.emulator.storagepath);
+        if (ret == 0 && def->tpm->data.emulator.logfile)
+            ret = _virSecuritySELinuxRestoreFileLabels(
+                mgr, def->tpm->data.emulator.logfile);
+        break;
+    case VIR_DOMAIN_TPM_TYPE_LAST:
+        break;
+    }
+
+    return ret;
+}
+
+
 virSecurityDriver virSecurityDriverSELinux = {
     .privateDataLen                     = sizeof(virSecuritySELinuxData),
     .name                               = SECURITY_SELINUX_NAME,
@@ -3107,4 +3268,7 @@ virSecurityDriver virSecurityDriverSELinux = {
 
     .domainSetSecurityChardevLabel      = virSecuritySELinuxSetChardevLabel,
     .domainRestoreSecurityChardevLabel  = virSecuritySELinuxRestoreChardevLabel,
+
+    .domainSetSecurityTPMLabels         = virSecuritySELinuxSetTPMLabels,
+    .domainRestoreSecurityTPMLabels     = virSecuritySELinuxRestoreTPMLabels,
 };
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 9615f9f..e37a681 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -760,6 +760,43 @@ virSecurityStackDomainRestoreChardevLabel(virSecurityManagerPtr mgr,
     return rc;
 }
 
+
+static int
+virSecurityStackSetTPMLabels(virSecurityManagerPtr mgr,
+                             virDomainDefPtr vm)
+{
+    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItemPtr item = priv->itemsHead;
+    int rc = 0;
+
+    for (; item; item = item->next) {
+        if (virSecurityManagerSetTPMLabels(item->securityManager,
+                                           vm) < 0)
+            rc = -1;
+    }
+
+    return rc;
+}
+
+
+static int
+virSecurityStackRestoreTPMLabels(virSecurityManagerPtr mgr,
+                                 virDomainDefPtr vm)
+{
+    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItemPtr item = priv->itemsHead;
+    int rc = 0;
+
+    for (; item; item = item->next) {
+        if (virSecurityManagerRestoreTPMLabels(item->securityManager,
+                                               vm) < 0)
+            rc = -1;
+    }
+
+    return rc;
+}
+
+
 virSecurityDriver virSecurityDriverStack = {
     .privateDataLen                     = sizeof(virSecurityStackData),
     .name                               = "stack",
@@ -822,4 +859,7 @@ virSecurityDriver virSecurityDriverStack = {
 
     .domainSetSecurityChardevLabel      = virSecurityStackDomainSetChardevLabel,
     .domainRestoreSecurityChardevLabel  = virSecurityStackDomainRestoreChardevLabel,
+
+    .domainSetSecurityTPMLabels         = virSecurityStackSetTPMLabels,
+    .domainRestoreSecurityTPMLabels     = virSecurityStackRestoreTPMLabels,
 };
-- 
2.5.5

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

On 05/10/2018 05:57 PM, Stefan Berger wrote:
> In this patch we label the swtpm process with SELinux labels. We give it the
> same label as the QEMU process has. We label its state directory and files
> as well. We restore the old security labels once the swtpm has terminated.
>
> The file and process labels now look as follows:
>
> Directory: /var/lib/libvirt/swtpm
>
> [root@localhost swtpm]# ls -lZ
> total 4
> rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm
>
> [root@localhost testvm]# ls -lZ
> total 8
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall
>
> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log
>
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]

An outstanding issue with this patch are the additional svirt rules that 
are needed. A challenge here is to find that subset of rules that are 
really needed.

One of my machines with Fedora 27 needs only this one rule (following 
permissive mode and audit2allow):

allow svirt_t swtpm_exec_t:file map;

The other FC27 machine needs these:

allow svirt_t bin_t:file entrypoint;
allow svirt_t swtpm_exec_t:file { entrypoint execute map read };

I have seen it needing several more rules than these.

I suppose it won't be a problem to get them accepted, assuming there 
will be an swtpm_exec_t...

    Stefan

>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>   src/libvirt_private.syms        |   2 +
>   src/qemu/qemu_tpm.c             |  24 +++++-
>   src/security/security_driver.h  |   7 ++
>   src/security/security_manager.c |  36 +++++++++
>   src/security/security_manager.h |   6 ++
>   src/security/security_selinux.c | 164 ++++++++++++++++++++++++++++++++++++++++
>   src/security/security_stack.c   |  40 ++++++++++
>   7 files changed, 278 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 75b8932..2ce67e7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>   virSecurityManagerRestoreInputLabel;
>   virSecurityManagerRestoreMemoryLabel;
>   virSecurityManagerRestoreSavedStateLabel;
> +virSecurityManagerRestoreTPMLabels;
>   virSecurityManagerSetAllLabel;
>   virSecurityManagerSetChardevLabel;
>   virSecurityManagerSetChildProcessLabel;
> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>   virSecurityManagerSetSavedStateLabel;
>   virSecurityManagerSetSocketLabel;
>   virSecurityManagerSetTapFDLabel;
> +virSecurityManagerSetTPMLabels;
>   virSecurityManagerStackAddNested;
>   virSecurityManagerTransactionAbort;
>   virSecurityManagerTransactionCommit;
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 024d24d..62f0146 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>
>       virCommandSetErrorBuffer(cmd, &errbuf);
>
> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
> +                                       def) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> +                                               def, cmd) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
> +        goto cleanup;
> +
> +    /* make sure we run this with the appropriate user */
> +    virCommandSetUID(cmd, cfg->swtpm_user);
> +    virCommandSetGID(cmd, cfg->swtpm_group);
> +
> +    ret = virCommandRun(cmd, &exitstatus);
> +
> +    virSecurityManagerPostFork(driver->securityManager);
> +
> +    if (ret < 0 || exitstatus != 0) {
>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>                       "stderr: %s"), exitstatus, errbuf);
>           virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -696,6 +715,8 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>       ret = 0;
>
>    cleanup:
> +    if (ret < 0)
> +        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
>       VIR_FREE(shortName);
>       VIR_FREE(errbuf);
>       virCommandFree(cmd);
> @@ -741,6 +762,7 @@ qemuExtTPMStop(virQEMUDriverPtr driver,
>               goto cleanup;
>
>           qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
> +        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
>           break;
>       case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
>       case VIR_DOMAIN_TPM_TYPE_LAST:
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 95e7c4d..cbf0ecf 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -149,6 +149,10 @@ typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr,
>                                                        virDomainDefPtr def,
>                                                        virDomainChrSourceDefPtr dev_source,
>                                                        bool chardevStdioLogd);
> +typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManagerPtr mgr,
> +                                              virDomainDefPtr def);
> +typedef int (*virSecurityDomainRestoreTPMLabels) (virSecurityManagerPtr mgr,
> +                                                  virDomainDefPtr def);
>
>
>   struct _virSecurityDriver {
> @@ -213,6 +217,9 @@ struct _virSecurityDriver {
>
>       virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel;
>       virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel;
> +
> +    virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels;
> +    virSecurityDomainRestoreTPMLabels domainRestoreSecurityTPMLabels;
>   };
>
>   virSecurityDriverPtr virSecurityDriverLookup(const char *name,
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 71f7f59..8683ad7 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -1204,3 +1204,39 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
>       virReportUnsupportedError();
>       return -1;
>   }
> +
> +
> +int
> +virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
> +                               virDomainDefPtr vm)
> +{
> +    int ret;
> +
> +    if (mgr->drv->domainSetSecurityTPMLabels) {
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm);
> +        virObjectUnlock(mgr);
> +
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +int
> +virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr vm)
> +{
> +    int ret;
> +
> +    if (mgr->drv->domainRestoreSecurityTPMLabels) {
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm);
> +        virObjectUnlock(mgr);
> +
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index c36a8b4..e772b61 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -194,4 +194,10 @@ int virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
>                                             virDomainChrSourceDefPtr dev_source,
>                                             bool chardevStdioLogd);
>
> +int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr vm);
> +
> +int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                       virDomainDefPtr vm);
> +
>   #endif /* VIR_SECURITY_MANAGER_H__ */
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 92e8415..6377fb7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -3048,6 +3048,167 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr,
>       return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel);
>   }
>
> +
> +/*
> + * _virSecuritySELinuxSetFileLabels:
> + *
> + * @mgr: the virSecurityManager
> + * @path: path to a directory or a file
> + * @seclabel: the security label
> + *
> + * Set the file labels on the given path; if the path is a directory
> + * we label all files found there, including the directory itself,
> + * otherwise we just label the file.
> + */
> +static int
> +_virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr,
> +                                 const char *path,
> +                                 virSecurityLabelDefPtr seclabel)
> +{
> +    int ret = 0;
> +    struct dirent *ent;
> +    char *filename = NULL;
> +    DIR *dir;
> +
> +    if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel)))
> +        return ret;
> +
> +    if (!virFileIsDir(path))
> +        return 0;
> +
> +    if (virDirOpen(&dir, path) < 0)
> +        return -1;
> +
> +    while ((ret = virDirRead(dir, &ent, path)) > 0) {
> +        if (ent->d_type != DT_REG)
> +            continue;
> +
> +        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
> +            ret = -1;
> +            break;
> +        }
> +        ret = virSecuritySELinuxSetFilecon(mgr, filename,
> +                                           seclabel->imagelabel);
> +        VIR_FREE(filename);
> +        if (ret < 0)
> +            break;
> +    }
> +    if (ret < 0)
> +        virReportSystemError(errno, _("Unable to label files under %s"),
> +                             path);
> +
> +    virDirClose(&dir);
> +
> +    return ret;
> +}
> +
> +
> +/*
> + * _virSecuritySELinuxRestoreFileLabels:
> + *
> + * @mgr: the virSecurityManager
> + * @path: path to a directory or a file
> + *
> + * Restore the file labels on the given path; if the path is a directory
> + * we restore all file labels found there, including the label of the
> + * directory itself, otherwise we just restore the label on the file.
> + */
> +static int
> +_virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr,
> +                                     const char *path)
> +{
> +    int ret = 0;
> +    struct dirent *ent;
> +    char *filename = NULL;
> +    DIR *dir;
> +
> +    if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path)))
> +        return ret;
> +
> +    if (!virFileIsDir(path))
> +        return 0;
> +
> +    if (virDirOpen(&dir, path) < 0)
> +        return -1;
> +
> +    while ((ret = virDirRead(dir, &ent, path)) > 0) {
> +        if (ent->d_type != DT_REG)
> +            continue;
> +
> +        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
> +            ret = -1;
> +            break;
> +        }
> +        ret = virSecuritySELinuxRestoreFileLabel(mgr, filename);
> +        VIR_FREE(filename);
> +        if (ret < 0)
> +            break;
> +    }
> +    if (ret < 0)
> +        virReportSystemError(errno, _("Unable to restore file labels under %s"),
> +                             path);
> +
> +    virDirClose(&dir);
> +
> +    return ret;
> +}
> +
> +
> +static int
> +virSecuritySELinuxSetTPMLabels(virSecurityManagerPtr mgr,
> +                               virDomainDefPtr def)
> +{
> +    int ret = 0;
> +    virSecurityLabelDefPtr seclabel;
> +
> +    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> +    if (seclabel == NULL)
> +        return 0;
> +
> +    switch (def->tpm->type) {
> +    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> +        ret = _virSecuritySELinuxSetFileLabels(
> +            mgr, def->tpm->data.emulator.storagepath,
> +            seclabel);
> +        if (ret == 0 && def->tpm->data.emulator.logfile)
> +            ret = _virSecuritySELinuxSetFileLabels(
> +                mgr, def->tpm->data.emulator.logfile,
> +                seclabel);
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_LAST:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +
> +static int
> +virSecuritySELinuxRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr def)
> +{
> +    int ret = 0;
> +
> +    switch (def->tpm->type) {
> +    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> +        ret = _virSecuritySELinuxRestoreFileLabels(
> +            mgr, def->tpm->data.emulator.storagepath);
> +        if (ret == 0 && def->tpm->data.emulator.logfile)
> +            ret = _virSecuritySELinuxRestoreFileLabels(
> +                mgr, def->tpm->data.emulator.logfile);
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_LAST:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +
>   virSecurityDriver virSecurityDriverSELinux = {
>       .privateDataLen                     = sizeof(virSecuritySELinuxData),
>       .name                               = SECURITY_SELINUX_NAME,
> @@ -3107,4 +3268,7 @@ virSecurityDriver virSecurityDriverSELinux = {
>
>       .domainSetSecurityChardevLabel      = virSecuritySELinuxSetChardevLabel,
>       .domainRestoreSecurityChardevLabel  = virSecuritySELinuxRestoreChardevLabel,
> +
> +    .domainSetSecurityTPMLabels         = virSecuritySELinuxSetTPMLabels,
> +    .domainRestoreSecurityTPMLabels     = virSecuritySELinuxRestoreTPMLabels,
>   };
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index 9615f9f..e37a681 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -760,6 +760,43 @@ virSecurityStackDomainRestoreChardevLabel(virSecurityManagerPtr mgr,
>       return rc;
>   }
>
> +
> +static int
> +virSecurityStackSetTPMLabels(virSecurityManagerPtr mgr,
> +                             virDomainDefPtr vm)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerSetTPMLabels(item->securityManager,
> +                                           vm) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
> +static int
> +virSecurityStackRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                 virDomainDefPtr vm)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerRestoreTPMLabels(item->securityManager,
> +                                               vm) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
>   virSecurityDriver virSecurityDriverStack = {
>       .privateDataLen                     = sizeof(virSecurityStackData),
>       .name                               = "stack",
> @@ -822,4 +859,7 @@ virSecurityDriver virSecurityDriverStack = {
>
>       .domainSetSecurityChardevLabel      = virSecurityStackDomainSetChardevLabel,
>       .domainRestoreSecurityChardevLabel  = virSecurityStackDomainRestoreChardevLabel,
> +
> +    .domainSetSecurityTPMLabels         = virSecurityStackSetTPMLabels,
> +    .domainRestoreSecurityTPMLabels     = virSecurityStackRestoreTPMLabels,
>   };


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

On 05/10/2018 11:57 PM, Stefan Berger wrote:
> In this patch we label the swtpm process with SELinux labels. We give it the
> same label as the QEMU process has. We label its state directory and files
> as well. We restore the old security labels once the swtpm has terminated.
> 
> The file and process labels now look as follows:
> 
> Directory: /var/lib/libvirt/swtpm
> 
> [root@localhost swtpm]# ls -lZ
> total 4
> rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm
> 
> [root@localhost testvm]# ls -lZ
> total 8
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall
> 
> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
> 
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log
> 
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
> 
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>   src/libvirt_private.syms        |   2 +
>   src/qemu/qemu_tpm.c             |  24 +++++-
>   src/security/security_driver.h  |   7 ++
>   src/security/security_manager.c |  36 +++++++++
>   src/security/security_manager.h |   6 ++
>   src/security/security_selinux.c | 164 ++++++++++++++++++++++++++++++++++++++++
>   src/security/security_stack.c   |  40 ++++++++++
>   7 files changed, 278 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 75b8932..2ce67e7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>   virSecurityManagerRestoreInputLabel;
>   virSecurityManagerRestoreMemoryLabel;
>   virSecurityManagerRestoreSavedStateLabel;
> +virSecurityManagerRestoreTPMLabels;
>   virSecurityManagerSetAllLabel;
>   virSecurityManagerSetChardevLabel;
>   virSecurityManagerSetChildProcessLabel;
> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>   virSecurityManagerSetSavedStateLabel;
>   virSecurityManagerSetSocketLabel;
>   virSecurityManagerSetTapFDLabel;
> +virSecurityManagerSetTPMLabels;

Shouldn't there be wrappers for
virSecurityManagerRestoreTPMLabels
virSecurityManagerSetTPMLabels
in src/qemu/qemu_security.h and possibly src/qemu/qemu_security.c?


>   virSecurityManagerStackAddNested;
>   virSecurityManagerTransactionAbort;
>   virSecurityManagerTransactionCommit;
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 024d24d..62f0146 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
make syntax-check error

0.03 prohibit_virConnectOpen_in_virsh
prohibit_virSecurity
../src/qemu/qemu_tpm.c:812:    if 
(virSecurityManagerSetTPMLabels(driver->securityManager,
../src/qemu/qemu_tpm.c:816:    if 
(virSecurityManagerSetChildProcessLabel(driver->securityManager,
../src/qemu/qemu_tpm.c:820:    if 
(virSecurityManagerPreFork(driver->securityManager) < 0)
../src/qemu/qemu_tpm.c:829: 
virSecurityManagerPostFork(driver->securityManager);
../src/qemu/qemu_tpm.c:860: 
virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
../src/qemu/qemu_tpm.c:911: 
virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
maint.mk: prefer qemuSecurity wrappers
../cfg.mk:998: recipe for target 'sc_prohibit_virSecurity' failed
make: *** [sc_prohibit_virSecurity] Error 1


> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>   
>       virCommandSetErrorBuffer(cmd, &errbuf);
>   
> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
> +                                       def) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> +                                               def, cmd) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
> +        goto cleanup;
> +
> +    /* make sure we run this with the appropriate user */
> +    virCommandSetUID(cmd, cfg->swtpm_user);
> +    virCommandSetGID(cmd, cfg->swtpm_group);
> +
> +    ret = virCommandRun(cmd, &exitstatus);
> +
> +    virSecurityManagerPostFork(driver->securityManager);
> +
> +    if (ret < 0 || exitstatus != 0) {
>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>                       "stderr: %s"), exitstatus, errbuf);
>           virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -696,6 +715,8 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>       ret = 0;
>   
>    cleanup:
> +    if (ret < 0)
> +        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
>       VIR_FREE(shortName);
>       VIR_FREE(errbuf);
>       virCommandFree(cmd);
> @@ -741,6 +762,7 @@ qemuExtTPMStop(virQEMUDriverPtr driver,
>               goto cleanup;
>   
>           qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
> +        virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
>           break;
>       case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
>       case VIR_DOMAIN_TPM_TYPE_LAST:
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 95e7c4d..cbf0ecf 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -149,6 +149,10 @@ typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr,
>                                                        virDomainDefPtr def,
>                                                        virDomainChrSourceDefPtr dev_source,
>                                                        bool chardevStdioLogd);
> +typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManagerPtr mgr,
> +                                              virDomainDefPtr def);
> +typedef int (*virSecurityDomainRestoreTPMLabels) (virSecurityManagerPtr mgr,
> +                                                  virDomainDefPtr def);
>   
>   
>   struct _virSecurityDriver {
> @@ -213,6 +217,9 @@ struct _virSecurityDriver {
>   
>       virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel;
>       virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel;
> +
> +    virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels;
> +    virSecurityDomainRestoreTPMLabels domainRestoreSecurityTPMLabels;
>   };
>   
>   virSecurityDriverPtr virSecurityDriverLookup(const char *name,
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 71f7f59..8683ad7 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -1204,3 +1204,39 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
>       virReportUnsupportedError();
>       return -1;
>   }
> +
> +
> +int
> +virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
> +                               virDomainDefPtr vm)
> +{
> +    int ret;
> +
> +    if (mgr->drv->domainSetSecurityTPMLabels) {
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainSetSecurityTPMLabels(mgr, vm);
> +        virObjectUnlock(mgr);
> +
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +int
> +virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr vm)
> +{
> +    int ret;
> +
> +    if (mgr->drv->domainRestoreSecurityTPMLabels) {
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainRestoreSecurityTPMLabels(mgr, vm);
> +        virObjectUnlock(mgr);
> +
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index c36a8b4..e772b61 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -194,4 +194,10 @@ int virSecurityManagerRestoreChardevLabel(virSecurityManagerPtr mgr,
>                                             virDomainChrSourceDefPtr dev_source,
>                                             bool chardevStdioLogd);
>   
> +int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr vm);
> +
> +int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                       virDomainDefPtr vm);
> +
>   #endif /* VIR_SECURITY_MANAGER_H__ */
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 92e8415..6377fb7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -3048,6 +3048,167 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr,
>       return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel);
>   }
>   
> +
> +/*
> + * _virSecuritySELinuxSetFileLabels:
> + *
> + * @mgr: the virSecurityManager
> + * @path: path to a directory or a file
> + * @seclabel: the security label
> + *
> + * Set the file labels on the given path; if the path is a directory
> + * we label all files found there, including the directory itself,
> + * otherwise we just label the file.
> + */
> +static int
> +_virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr,
> +                                 const char *path,
> +                                 virSecurityLabelDefPtr seclabel)
> +{
> +    int ret = 0;
> +    struct dirent *ent;
> +    char *filename = NULL;
> +    DIR *dir;
> +
> +    if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel)))
> +        return ret;
> +
> +    if (!virFileIsDir(path))
> +        return 0;
> +
> +    if (virDirOpen(&dir, path) < 0)
> +        return -1;
> +
> +    while ((ret = virDirRead(dir, &ent, path)) > 0) {
> +        if (ent->d_type != DT_REG)
> +            continue;
> +
> +        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
> +            ret = -1;
> +            break;
> +        }
> +        ret = virSecuritySELinuxSetFilecon(mgr, filename,
> +                                           seclabel->imagelabel);
> +        VIR_FREE(filename);
> +        if (ret < 0)
> +            break;
> +    }
> +    if (ret < 0)
> +        virReportSystemError(errno, _("Unable to label files under %s"),
> +                             path);
> +
> +    virDirClose(&dir);
> +
> +    return ret;
> +}
> +
> +
> +/*
> + * _virSecuritySELinuxRestoreFileLabels:
> + *
> + * @mgr: the virSecurityManager
> + * @path: path to a directory or a file
> + *
> + * Restore the file labels on the given path; if the path is a directory
> + * we restore all file labels found there, including the label of the
> + * directory itself, otherwise we just restore the label on the file.
> + */
> +static int
> +_virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr,
> +                                     const char *path)
> +{
> +    int ret = 0;
> +    struct dirent *ent;
> +    char *filename = NULL;
> +    DIR *dir;
> +
> +    if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path)))
> +        return ret;
> +
> +    if (!virFileIsDir(path))
> +        return 0;
> +
> +    if (virDirOpen(&dir, path) < 0)
> +        return -1;
> +
> +    while ((ret = virDirRead(dir, &ent, path)) > 0) {
> +        if (ent->d_type != DT_REG)
> +            continue;
> +
> +        if (virAsprintf(&filename, "%s/%s", path, ent->d_name) < 0) {
> +            ret = -1;
> +            break;
> +        }
> +        ret = virSecuritySELinuxRestoreFileLabel(mgr, filename);
> +        VIR_FREE(filename);
> +        if (ret < 0)
> +            break;
> +    }
> +    if (ret < 0)
> +        virReportSystemError(errno, _("Unable to restore file labels under %s"),
> +                             path);
> +
> +    virDirClose(&dir);
> +
> +    return ret;
> +}
> +
> +
> +static int
> +virSecuritySELinuxSetTPMLabels(virSecurityManagerPtr mgr,
> +                               virDomainDefPtr def)
> +{
> +    int ret = 0;
> +    virSecurityLabelDefPtr seclabel;
> +
> +    seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> +    if (seclabel == NULL)
> +        return 0;
> +
> +    switch (def->tpm->type) {
> +    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> +        ret = _virSecuritySELinuxSetFileLabels(
> +            mgr, def->tpm->data.emulator.storagepath,
> +            seclabel);
> +        if (ret == 0 && def->tpm->data.emulator.logfile)
> +            ret = _virSecuritySELinuxSetFileLabels(
> +                mgr, def->tpm->data.emulator.logfile,
> +                seclabel);
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_LAST:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +
> +static int
> +virSecuritySELinuxRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                   virDomainDefPtr def)
> +{
> +    int ret = 0;
> +
> +    switch (def->tpm->type) {
> +    case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> +        ret = _virSecuritySELinuxRestoreFileLabels(
> +            mgr, def->tpm->data.emulator.storagepath);
> +        if (ret == 0 && def->tpm->data.emulator.logfile)
> +            ret = _virSecuritySELinuxRestoreFileLabels(
> +                mgr, def->tpm->data.emulator.logfile);
> +        break;
> +    case VIR_DOMAIN_TPM_TYPE_LAST:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +
>   virSecurityDriver virSecurityDriverSELinux = {
>       .privateDataLen                     = sizeof(virSecuritySELinuxData),
>       .name                               = SECURITY_SELINUX_NAME,
> @@ -3107,4 +3268,7 @@ virSecurityDriver virSecurityDriverSELinux = {
>   
>       .domainSetSecurityChardevLabel      = virSecuritySELinuxSetChardevLabel,
>       .domainRestoreSecurityChardevLabel  = virSecuritySELinuxRestoreChardevLabel,
> +
> +    .domainSetSecurityTPMLabels         = virSecuritySELinuxSetTPMLabels,
> +    .domainRestoreSecurityTPMLabels     = virSecuritySELinuxRestoreTPMLabels,
>   };
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index 9615f9f..e37a681 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -760,6 +760,43 @@ virSecurityStackDomainRestoreChardevLabel(virSecurityManagerPtr mgr,
>       return rc;
>   }
>   
> +
> +static int
> +virSecurityStackSetTPMLabels(virSecurityManagerPtr mgr,
> +                             virDomainDefPtr vm)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerSetTPMLabels(item->securityManager,
> +                                           vm) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
> +static int
> +virSecurityStackRestoreTPMLabels(virSecurityManagerPtr mgr,
> +                                 virDomainDefPtr vm)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerRestoreTPMLabels(item->securityManager,
> +                                               vm) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
>   virSecurityDriver virSecurityDriverStack = {
>       .privateDataLen                     = sizeof(virSecurityStackData),
>       .name                               = "stack",
> @@ -822,4 +859,7 @@ virSecurityDriver virSecurityDriverStack = {
>   
>       .domainSetSecurityChardevLabel      = virSecurityStackDomainSetChardevLabel,
>       .domainRestoreSecurityChardevLabel  = virSecurityStackDomainRestoreChardevLabel,
> +
> +    .domainSetSecurityTPMLabels         = virSecurityStackSetTPMLabels,
> +    .domainRestoreSecurityTPMLabels     = virSecurityStackRestoreTPMLabels,
>   };
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

On 05/15/2018 06:30 AM, Boris Fiuczynski wrote:
> On 05/10/2018 11:57 PM, Stefan Berger wrote:
>> In this patch we label the swtpm process with SELinux labels. We give 
>> it the
>> same label as the QEMU process has. We label its state directory and 
>> files
>> as well. We restore the old security labels once the swtpm has 
>> terminated.
>>
>> The file and process labels now look as follows:
>>
>> Directory: /var/lib/libvirt/swtpm
>>
>> [root@localhost swtpm]# ls -lZ
>> total 4
>> rwx------. 2 tss  tss system_u:object_r:svirt_image_t:s0:c254,c932 
>> 4096 Apr  5 16:46 testvm
>>
>> [root@localhost testvm]# ls -lZ
>> total 8
>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>> 3648 Apr  5 16:46 tpm-00.permall
>>
>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>
>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>> 2237 Apr  5 16:46 vtpm.log
>>
>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep 
>> swtpm | grep ctrl | grep -v grep
>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172 3892 
>> ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl 
>> type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 
>> --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log 
>> file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>
>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep 
>> qemu | grep tpm | grep -v grep
>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 
>> 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   src/libvirt_private.syms        |   2 +
>>   src/qemu/qemu_tpm.c             |  24 +++++-
>>   src/security/security_driver.h  |   7 ++
>>   src/security/security_manager.c |  36 +++++++++
>>   src/security/security_manager.h |   6 ++
>>   src/security/security_selinux.c | 164 
>> ++++++++++++++++++++++++++++++++++++++++
>>   src/security/security_stack.c   |  40 ++++++++++
>>   7 files changed, 278 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 75b8932..2ce67e7 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>   virSecurityManagerRestoreInputLabel;
>>   virSecurityManagerRestoreMemoryLabel;
>>   virSecurityManagerRestoreSavedStateLabel;
>> +virSecurityManagerRestoreTPMLabels;
>>   virSecurityManagerSetAllLabel;
>>   virSecurityManagerSetChardevLabel;
>>   virSecurityManagerSetChildProcessLabel;
>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>   virSecurityManagerSetSavedStateLabel;
>>   virSecurityManagerSetSocketLabel;
>>   virSecurityManagerSetTapFDLabel;
>> +virSecurityManagerSetTPMLabels;
>
> Shouldn't there be wrappers for
> virSecurityManagerRestoreTPMLabels
> virSecurityManagerSetTPMLabels
> in src/qemu/qemu_security.h and possibly src/qemu/qemu_security.c?
>
>
>>   virSecurityManagerStackAddNested;
>>   virSecurityManagerTransactionAbort;
>>   virSecurityManagerTransactionCommit;
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index 024d24d..62f0146 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
> make syntax-check error
>
> 0.03 prohibit_virConnectOpen_in_virsh
> prohibit_virSecurity
> ../src/qemu/qemu_tpm.c:812:    if 
> (virSecurityManagerSetTPMLabels(driver->securityManager,
> ../src/qemu/qemu_tpm.c:816:    if 
> (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> ../src/qemu/qemu_tpm.c:820:    if 
> (virSecurityManagerPreFork(driver->securityManager) < 0)
> ../src/qemu/qemu_tpm.c:829: 
> virSecurityManagerPostFork(driver->securityManager);
> ../src/qemu/qemu_tpm.c:860: 
> virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
> ../src/qemu/qemu_tpm.c:911: 
> virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
> maint.mk: prefer qemuSecurity wrappers
> ../cfg.mk:998: recipe for target 'sc_prohibit_virSecurity' failed
> make: *** [sc_prohibit_virSecurity] Error 1

I wrapped this now in two functions:

int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
                                  virDomainDefPtr def,
                                  virCommandPtr cmd,
                                  uid_t uid,
                                  gid_t gid,
                                  int *exitstatus,
                                  int *cmdret);

void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver,
                                     virDomainDefPtr def);

I will repost a v5 later today.

    Stefan

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

On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> In this patch we label the swtpm process with SELinux labels. We give it the
> same label as the QEMU process has. We label its state directory and files
> as well. We restore the old security labels once the swtpm has terminated.
>
> The file and process labels now look as follows:
>
> Directory: /var/lib/libvirt/swtpm
>
> [root@localhost swtpm]# ls -lZ
> total 4
> rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm
>
> [root@localhost testvm]# ls -lZ
> total 8
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall
>
> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>
> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log
>
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>
> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  src/libvirt_private.syms        |   2 +
>  src/qemu/qemu_tpm.c             |  24 +++++-
>  src/security/security_driver.h  |   7 ++
>  src/security/security_manager.c |  36 +++++++++
>  src/security/security_manager.h |   6 ++
>  src/security/security_selinux.c | 164 ++++++++++++++++++++++++++++++++++++++++
>  src/security/security_stack.c   |  40 ++++++++++
>  7 files changed, 278 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 75b8932..2ce67e7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>  virSecurityManagerRestoreInputLabel;
>  virSecurityManagerRestoreMemoryLabel;
>  virSecurityManagerRestoreSavedStateLabel;
> +virSecurityManagerRestoreTPMLabels;
>  virSecurityManagerSetAllLabel;
>  virSecurityManagerSetChardevLabel;
>  virSecurityManagerSetChildProcessLabel;
> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>  virSecurityManagerSetSavedStateLabel;
>  virSecurityManagerSetSocketLabel;
>  virSecurityManagerSetTapFDLabel;
> +virSecurityManagerSetTPMLabels;
>  virSecurityManagerStackAddNested;
>  virSecurityManagerTransactionAbort;
>  virSecurityManagerTransactionCommit;
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 024d24d..62f0146 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>
>      virCommandSetErrorBuffer(cmd, &errbuf);
>
> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
> +                                       def) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> +                                               def, cmd) < 0)
> +        goto cleanup;
> +
> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
> +        goto cleanup;
> +
> +    /* make sure we run this with the appropriate user */
> +    virCommandSetUID(cmd, cfg->swtpm_user);
> +    virCommandSetGID(cmd, cfg->swtpm_group);
> +
> +    ret = virCommandRun(cmd, &exitstatus);
> +
> +    virSecurityManagerPostFork(driver->securityManager);
> +
> +    if (ret < 0 || exitstatus != 0) {
>          VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>                      "stderr: %s"), exitstatus, errbuf);
>          virReportError(VIR_ERR_INTERNAL_ERROR,

Don't we have to set ret to -1 here (for the case where ret == 0 &&
exitstatus != 0)?

[…snip]

Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

On 05/15/2018 11:38 AM, Marc Hartmayer wrote:
> On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>> In this patch we label the swtpm process with SELinux labels. We give it the
>> same label as the QEMU process has. We label its state directory and files
>> as well. We restore the old security labels once the swtpm has terminated.
>>
>> The file and process labels now look as follows:
>>
>> Directory: /var/lib/libvirt/swtpm
>>
>> [root@localhost swtpm]# ls -lZ
>> total 4
>> rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm
>>
>> [root@localhost testvm]# ls -lZ
>> total 8
>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall
>>
>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>
>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log
>>
>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>
>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   src/libvirt_private.syms        |   2 +
>>   src/qemu/qemu_tpm.c             |  24 +++++-
>>   src/security/security_driver.h  |   7 ++
>>   src/security/security_manager.c |  36 +++++++++
>>   src/security/security_manager.h |   6 ++
>>   src/security/security_selinux.c | 164 ++++++++++++++++++++++++++++++++++++++++
>>   src/security/security_stack.c   |  40 ++++++++++
>>   7 files changed, 278 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 75b8932..2ce67e7 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>   virSecurityManagerRestoreInputLabel;
>>   virSecurityManagerRestoreMemoryLabel;
>>   virSecurityManagerRestoreSavedStateLabel;
>> +virSecurityManagerRestoreTPMLabels;
>>   virSecurityManagerSetAllLabel;
>>   virSecurityManagerSetChardevLabel;
>>   virSecurityManagerSetChildProcessLabel;
>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>   virSecurityManagerSetSavedStateLabel;
>>   virSecurityManagerSetSocketLabel;
>>   virSecurityManagerSetTapFDLabel;
>> +virSecurityManagerSetTPMLabels;
>>   virSecurityManagerStackAddNested;
>>   virSecurityManagerTransactionAbort;
>>   virSecurityManagerTransactionCommit;
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index 024d24d..62f0146 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
>> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>>
>>       virCommandSetErrorBuffer(cmd, &errbuf);
>>
>> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
>> +                                       def) < 0)
>> +        goto cleanup;
>> +
>> +    if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
>> +                                               def, cmd) < 0)
>> +        goto cleanup;
>> +
>> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
>> +        goto cleanup;
>> +
>> +    /* make sure we run this with the appropriate user */
>> +    virCommandSetUID(cmd, cfg->swtpm_user);
>> +    virCommandSetGID(cmd, cfg->swtpm_group);
>> +
>> +    ret = virCommandRun(cmd, &exitstatus);
>> +
>> +    virSecurityManagerPostFork(driver->securityManager);
>> +
>> +    if (ret < 0 || exitstatus != 0) {
>>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>>                       "stderr: %s"), exitstatus, errbuf);
>>           virReportError(VIR_ERR_INTERNAL_ERROR,
> Don't we have to set ret to -1 here (for the case where ret == 0 &&
> exitstatus != 0)?

Very true.

   Stefan

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

On 05/15/2018 11:45 AM, Stefan Berger wrote:
> On 05/15/2018 11:38 AM, Marc Hartmayer wrote:
>> On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger 
>> <stefanb@linux.vnet.ibm.com> wrote:
>>> In this patch we label the swtpm process with SELinux labels. We 
>>> give it the
>>> same label as the QEMU process has. We label its state directory and 
>>> files
>>> as well. We restore the old security labels once the swtpm has 
>>> terminated.
>>>
>>> The file and process labels now look as follows:
>>>
>>> Directory: /var/lib/libvirt/swtpm
>>>
>>> [root@localhost swtpm]# ls -lZ
>>> total 4
>>> rwx------. 2 tss  tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 4096 Apr  5 16:46 testvm
>>>
>>> [root@localhost testvm]# ls -lZ
>>> total 8
>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 3648 Apr  5 16:46 tpm-00.permall
>>>
>>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>>
>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 
>>> 2237 Apr  5 16:46 vtpm.log
>>>
>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | 
>>> grep swtpm | grep ctrl | grep -v grep
>>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  
>>> 3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon 
>>> --ctrl 
>>> type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 
>>> --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log 
>>> file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>>
>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | 
>>> grep qemu | grep tpm | grep -v grep
>>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 
>>> 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> ---
>>>   src/libvirt_private.syms        |   2 +
>>>   src/qemu/qemu_tpm.c             |  24 +++++-
>>>   src/security/security_driver.h  |   7 ++
>>>   src/security/security_manager.c |  36 +++++++++
>>>   src/security/security_manager.h |   6 ++
>>>   src/security/security_selinux.c | 164 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   src/security/security_stack.c   |  40 ++++++++++
>>>   7 files changed, 278 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 75b8932..2ce67e7 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>>   virSecurityManagerRestoreInputLabel;
>>>   virSecurityManagerRestoreMemoryLabel;
>>>   virSecurityManagerRestoreSavedStateLabel;
>>> +virSecurityManagerRestoreTPMLabels;
>>>   virSecurityManagerSetAllLabel;
>>>   virSecurityManagerSetChardevLabel;
>>>   virSecurityManagerSetChildProcessLabel;
>>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>>   virSecurityManagerSetSavedStateLabel;
>>>   virSecurityManagerSetSocketLabel;
>>>   virSecurityManagerSetTapFDLabel;
>>> +virSecurityManagerSetTPMLabels;
>>>   virSecurityManagerStackAddNested;
>>>   virSecurityManagerTransactionAbort;
>>>   virSecurityManagerTransactionCommit;
>>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>>> index 024d24d..62f0146 100644
>>> --- a/src/qemu/qemu_tpm.c
>>> +++ b/src/qemu/qemu_tpm.c
>>> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>>>
>>>       virCommandSetErrorBuffer(cmd, &errbuf);
>>>
>>> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>>> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
>>> +                                       def) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if 
>>> (virSecurityManagerSetChildProcessLabel(driver->securityManager,
>>> +                                               def, cmd) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
>>> +        goto cleanup;
>>> +
>>> +    /* make sure we run this with the appropriate user */
>>> +    virCommandSetUID(cmd, cfg->swtpm_user);
>>> +    virCommandSetGID(cmd, cfg->swtpm_group);
>>> +
>>> +    ret = virCommandRun(cmd, &exitstatus);
>>> +
>>> +    virSecurityManagerPostFork(driver->securityManager);
>>> +
>>> +    if (ret < 0 || exitstatus != 0) {
>>>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>>>                       "stderr: %s"), exitstatus, errbuf);
>>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>> Don't we have to set ret to -1 here (for the case where ret == 0 &&
>> exitstatus != 0)?
>
> Very true.

ret is initialized with '-1'. So we are good.

    Stefan

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

On Tue, May 15, 2018 at 05:50 PM +0200, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> On 05/15/2018 11:45 AM, Stefan Berger wrote:
>> On 05/15/2018 11:38 AM, Marc Hartmayer wrote:
>>> On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> In this patch we label the swtpm process with SELinux labels. We
>>>> give it the
>>>> same label as the QEMU process has. We label its state directory and
>>>> files
>>>> as well. We restore the old security labels once the swtpm has
>>>> terminated.
>>>>
>>>> The file and process labels now look as follows:
>>>>
>>>> Directory: /var/lib/libvirt/swtpm
>>>>
>>>> [root@localhost swtpm]# ls -lZ
>>>> total 4
>>>> rwx------. 2 tss  tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>> 4096 Apr  5 16:46 testvm
>>>>
>>>> [root@localhost testvm]# ls -lZ
>>>> total 8
>>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>> 3648 Apr  5 16:46 tpm-00.permall
>>>>
>>>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>>>
>>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>> 2237 Apr  5 16:46 vtpm.log
>>>>
>>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ |
>>>> grep swtpm | grep ctrl | grep -v grep
>>>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172
>>>> 3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon
>>>> --ctrl
>>>> type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660
>>>> --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log
>>>> file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>>>
>>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ |
>>>> grep qemu | grep tpm | grep -v grep
>>>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704
>>>> 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>> ---
>>>>   src/libvirt_private.syms        |   2 +
>>>>   src/qemu/qemu_tpm.c             |  24 +++++-
>>>>   src/security/security_driver.h  |   7 ++
>>>>   src/security/security_manager.c |  36 +++++++++
>>>>   src/security/security_manager.h |   6 ++
>>>>   src/security/security_selinux.c | 164
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>   src/security/security_stack.c   |  40 ++++++++++
>>>>   7 files changed, 278 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>>> index 75b8932..2ce67e7 100644
>>>> --- a/src/libvirt_private.syms
>>>> +++ b/src/libvirt_private.syms
>>>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>>>   virSecurityManagerRestoreInputLabel;
>>>>   virSecurityManagerRestoreMemoryLabel;
>>>>   virSecurityManagerRestoreSavedStateLabel;
>>>> +virSecurityManagerRestoreTPMLabels;
>>>>   virSecurityManagerSetAllLabel;
>>>>   virSecurityManagerSetChardevLabel;
>>>>   virSecurityManagerSetChildProcessLabel;
>>>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>>>   virSecurityManagerSetSavedStateLabel;
>>>>   virSecurityManagerSetSocketLabel;
>>>>   virSecurityManagerSetTapFDLabel;
>>>> +virSecurityManagerSetTPMLabels;
>>>>   virSecurityManagerStackAddNested;
>>>>   virSecurityManagerTransactionAbort;
>>>>   virSecurityManagerTransactionCommit;
>>>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>>>> index 024d24d..62f0146 100644
>>>> --- a/src/qemu/qemu_tpm.c
>>>> +++ b/src/qemu/qemu_tpm.c
>>>> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>>>>
>>>>       virCommandSetErrorBuffer(cmd, &errbuf);
>>>>
>>>> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>>>> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
>>>> +                                       def) < 0)
>>>> +        goto cleanup;
>>>> +
>>>> +    if
>>>> (virSecurityManagerSetChildProcessLabel(driver->securityManager,
>>>> +                                               def, cmd) < 0)
>>>> +        goto cleanup;
>>>> +
>>>> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
>>>> +        goto cleanup;
>>>> +
>>>> +    /* make sure we run this with the appropriate user */
>>>> +    virCommandSetUID(cmd, cfg->swtpm_user);
>>>> +    virCommandSetGID(cmd, cfg->swtpm_group);
>>>> +
>>>> +    ret = virCommandRun(cmd, &exitstatus);
>>>> +
>>>> +    virSecurityManagerPostFork(driver->securityManager);
>>>> +
>>>> +    if (ret < 0 || exitstatus != 0) {
>>>>           VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>>>>                       "stderr: %s"), exitstatus, errbuf);
>>>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>>> Don't we have to set ret to -1 here (for the case where ret == 0 &&
>>> exitstatus != 0)?
>>
>> Very true.
>
> ret is initialized with '-1'. So we are good.

No we are not good since @ret is overwritten with:

ret = virCommandRun(cmd, &exitstatus);

So it’s possible that virCommandRun returns 0 but exitstatus != 0
=> ret == 0; exitstatus != 0
=> virReportError(…) reports an error but the return value @ret remains 0.

>
>     Stefan

Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

On 05/15/2018 12:13 PM, Marc Hartmayer wrote:
> On Tue, May 15, 2018 at 05:50 PM +0200, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>> On 05/15/2018 11:45 AM, Stefan Berger wrote:
>>> On 05/15/2018 11:38 AM, Marc Hartmayer wrote:
>>>> On Thu, May 10, 2018 at 11:57 PM +0200, Stefan Berger
>>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>>> In this patch we label the swtpm process with SELinux labels. We
>>>>> give it the
>>>>> same label as the QEMU process has. We label its state directory and
>>>>> files
>>>>> as well. We restore the old security labels once the swtpm has
>>>>> terminated.
>>>>>
>>>>> The file and process labels now look as follows:
>>>>>
>>>>> Directory: /var/lib/libvirt/swtpm
>>>>>
>>>>> [root@localhost swtpm]# ls -lZ
>>>>> total 4
>>>>> rwx------. 2 tss  tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>>> 4096 Apr  5 16:46 testvm
>>>>>
>>>>> [root@localhost testvm]# ls -lZ
>>>>> total 8
>>>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>>> 3648 Apr  5 16:46 tpm-00.permall
>>>>>
>>>>> The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
>>>>>
>>>>> -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932
>>>>> 2237 Apr  5 16:46 vtpm.log
>>>>>
>>>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ |
>>>>> grep swtpm | grep ctrl | grep -v grep
>>>>> system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172
>>>>> 3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon
>>>>> --ctrl
>>>>> type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660
>>>>> --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log
>>>>> file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
>>>>>
>>>>> [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ |
>>>>> grep qemu | grep tpm | grep -v grep
>>>>> system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704
>>>>> 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
>>>>>
>>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>>> ---
>>>>>    src/libvirt_private.syms        |   2 +
>>>>>    src/qemu/qemu_tpm.c             |  24 +++++-
>>>>>    src/security/security_driver.h  |   7 ++
>>>>>    src/security/security_manager.c |  36 +++++++++
>>>>>    src/security/security_manager.h |   6 ++
>>>>>    src/security/security_selinux.c | 164
>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>    src/security/security_stack.c   |  40 ++++++++++
>>>>>    7 files changed, 278 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>>>> index 75b8932..2ce67e7 100644
>>>>> --- a/src/libvirt_private.syms
>>>>> +++ b/src/libvirt_private.syms
>>>>> @@ -1319,6 +1319,7 @@ virSecurityManagerRestoreImageLabel;
>>>>>    virSecurityManagerRestoreInputLabel;
>>>>>    virSecurityManagerRestoreMemoryLabel;
>>>>>    virSecurityManagerRestoreSavedStateLabel;
>>>>> +virSecurityManagerRestoreTPMLabels;
>>>>>    virSecurityManagerSetAllLabel;
>>>>>    virSecurityManagerSetChardevLabel;
>>>>>    virSecurityManagerSetChildProcessLabel;
>>>>> @@ -1333,6 +1334,7 @@ virSecurityManagerSetProcessLabel;
>>>>>    virSecurityManagerSetSavedStateLabel;
>>>>>    virSecurityManagerSetSocketLabel;
>>>>>    virSecurityManagerSetTapFDLabel;
>>>>> +virSecurityManagerSetTPMLabels;
>>>>>    virSecurityManagerStackAddNested;
>>>>>    virSecurityManagerTransactionAbort;
>>>>>    virSecurityManagerTransactionCommit;
>>>>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>>>>> index 024d24d..62f0146 100644
>>>>> --- a/src/qemu/qemu_tpm.c
>>>>> +++ b/src/qemu/qemu_tpm.c
>>>>> @@ -684,7 +684,26 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>>>>>
>>>>>        virCommandSetErrorBuffer(cmd, &errbuf);
>>>>>
>>>>> -    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>>>>> +    if (virSecurityManagerSetTPMLabels(driver->securityManager,
>>>>> +                                       def) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    if
>>>>> (virSecurityManagerSetChildProcessLabel(driver->securityManager,
>>>>> +                                               def, cmd) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    if (virSecurityManagerPreFork(driver->securityManager) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    /* make sure we run this with the appropriate user */
>>>>> +    virCommandSetUID(cmd, cfg->swtpm_user);
>>>>> +    virCommandSetGID(cmd, cfg->swtpm_group);
>>>>> +
>>>>> +    ret = virCommandRun(cmd, &exitstatus);
>>>>> +
>>>>> +    virSecurityManagerPostFork(driver->securityManager);
>>>>> +
>>>>> +    if (ret < 0 || exitstatus != 0) {
>>>>>            VIR_ERROR(_("Could not start 'swtpm'. exitstatus: %d "
>>>>>                        "stderr: %s"), exitstatus, errbuf);
>>>>>            virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> Don't we have to set ret to -1 here (for the case where ret == 0 &&
>>>> exitstatus != 0)?
>>> Very true.
>> ret is initialized with '-1'. So we are good.
> No we are not good since @ret is overwritten with:
>
> ret = virCommandRun(cmd, &exitstatus);
>
> So it’s possible that virCommandRun returns 0 but exitstatus != 0
> => ret == 0; exitstatus != 0
> => virReportError(…) reports an error but the return value @ret remains 0.

After the modifications the patch looks like this and doesn't touch 
'ret' anymore:

@@ -684,7 +686,12 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,

      virCommandSetErrorBuffer(cmd, &errbuf);

-    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
+    if (qemuSecurityStartTPMEmulator(driver, def, cmd,
+                                     cfg->swtpm_user, cfg->swtpm_group,
+                                     &exitstatus, &cmdret) < 0)
+        goto cleanup;
+
+    if (cmdret < 0 || exitstatus != 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR,
                         _("Could not start 'swtpm'. exitstatus: %d, "
                         "error: %s"), exitstatus, errbuf);


     Stefan

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