[libvirt] [PATCH 0/3] security: Don't remember labels for TPM
As it turns out, /dev/tpm0 can't be opened more than once. This doesn't fit into our seclabel remembering approach and thus disable it for TPM devices. There's also another type of files which can't be opened more than once - /dev/vfio/N. Usually, this won't be a problem unless users try to attach/detach some devices from the same IOMMU group. This will require more treatment though because it's broken on more levels. 1) we remove /dev/vfio/N in private devtmpfs on device detach, even though there is another device still attached to domain from the same IOMMU group, 2) we remove the IOMMU group from CGroups, i.e. we effectively deny access to qemu 3) we restore seclabels (regardless of seclabel remembering) Therefore, I'm only addressing TPM issue here and will continue work on hostdevs. Michal Prívozník (3): security: Try to lock only paths with remember == true security_dac: Allow selective remember/recall for chardevs security: Don't remember labels for TPM src/security/security_dac.c | 91 ++++++++++++++++++++++----------- src/security/security_selinux.c | 16 +++--- 2 files changed, 71 insertions(+), 36 deletions(-) -- 2.21.0
So far all items on the chown/setfilecon list have the same .remember value. But this will change shortly. Therefore, don't try to lock paths which we won't manipulate XATTRs for. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 6 ++++-- src/security/security_selinux.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b4afef18a..5df50bdcf5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -232,9 +232,11 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, return -1; for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + virSecurityDACChownItemPtr item = list->items[i]; + const char *p = item->path; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + if (item->remember) + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e879fa39ab..e3be724a2b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -266,9 +266,11 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, return -1; for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + virSecuritySELinuxContextItemPtr item = list->items[i]; + const char *p = item->path; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + if (item->remember) + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) -- 2.21.0
While in most cases we want to remember/recall label for a chardev, there are some special ones (like /dev/tpm0) where we don't want to remember the seclabel nor recall it. See next commit for rationale behind. While the easiest way to implement this would be to just add new argument to virSecurityDACSetChardevLabel() this one is also a callback for virSecurityManagerSetChardevLabel() and thus has more or less stable set of arguments. Therefore, the current virSecurityDACSetChardevLabel() is renamed to virSecurityDACSetChardevLabelHelper() and the original function is set to call the new one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 67 +++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 5df50bdcf5..2733fa664f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1431,10 +1431,11 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, static int -virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainChrSourceDefPtr dev_source, - bool chardevStdioLogd) +virSecurityDACSetChardevLabelHelper(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd, + bool remember) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); @@ -1471,7 +1472,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, true); + user, group, remember); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1479,12 +1480,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, true) < 0 || - virSecurityDACSetOwnership(mgr, NULL, out, user, group, true) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, remember) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group, remember) < 0) goto done; } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, true) < 0) { + user, group, remember) < 0) { goto done; } ret = 0; @@ -1499,7 +1500,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, - user, group, true) < 0) + user, group, remember) < 0) goto done; } ret = 0; @@ -1525,11 +1526,24 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, return ret; } + static int -virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def ATTRIBUTE_UNUSED, - virDomainChrSourceDefPtr dev_source, - bool chardevStdioLogd) +virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + return virSecurityDACSetChardevLabelHelper(mgr, def, dev_source, + chardevStdioLogd, true); +} + + +static int +virSecurityDACRestoreChardevLabelHelper(virSecurityManagerPtr mgr, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd, + bool recall) { virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; @@ -1549,7 +1563,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType)dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path); + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL, + dev_source->data.file.path, + recall); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1557,10 +1573,12 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACRestoreFileLabel(mgr, out) < 0 || - virSecurityDACRestoreFileLabel(mgr, in) < 0) + if (virSecurityDACRestoreFileLabelInternal(mgr, NULL, out, recall) < 0 || + virSecurityDACRestoreFileLabelInternal(mgr, NULL, in, recall) < 0) goto done; - } else if (virSecurityDACRestoreFileLabel(mgr, dev_source->data.file.path) < 0) { + } else if (virSecurityDACRestoreFileLabelInternal(mgr, NULL, + dev_source->data.file.path, + recall) < 0) { goto done; } ret = 0; @@ -1568,7 +1586,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen && - virSecurityDACRestoreFileLabel(mgr, dev_source->data.nix.path) < 0) { + virSecurityDACRestoreFileLabelInternal(mgr, NULL, + dev_source->data.nix.path, + recall) < 0) { goto done; } ret = 0; @@ -1595,6 +1615,17 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, } +static int +virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd) +{ + return virSecurityDACRestoreChardevLabelHelper(mgr, def, dev_source, + chardevStdioLogd, true); +} + + struct _virSecuritySELinuxChardevCallbackData { virSecurityManagerPtr mgr; bool chardevStdioLogd; -- 2.21.0
https://bugzilla.redhat.com/show_bug.cgi?id=1755803 The /dev/tpmN file can be opened only once, as implemented in drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any other attempt to open the file fails. And since we're opening the file ourselves and passing the FD to qemu we will not succeed opening the file again when locking it for seclabel remembering. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 18 +++++++++--------- src/security/security_selinux.c | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2733fa664f..347a7a5f63 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1653,14 +1653,14 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.emulator.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.emulator.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1679,9 +1679,9 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACRestoreChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: /* swtpm will have removed the Unix socket upon termination */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e3be724a2b..0486bdd6b6 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1682,14 +1682,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel, true); + seclabel->imagelabel, false); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1701,7 +1701,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; break; @@ -1730,10 +1730,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, true); + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { - if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, true) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) rc = -1; VIR_FREE(cancel_path); } -- 2.21.0
On 10/1/19 11:00 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1755803
The /dev/tpmN file can be opened only once, as implemented in drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any other attempt to open the file fails. And since we're opening the file ourselves and passing the FD to qemu we will not succeed opening the file again when locking it for seclabel remembering.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 18 +++++++++--------- src/security/security_selinux.c | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2733fa664f..347a7a5f63 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1653,14 +1653,14 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.emulator.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.emulator.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1679,9 +1679,9 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACRestoreChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: /* swtpm will have removed the Unix socket upon termination */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e3be724a2b..0486bdd6b6 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1682,14 +1682,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1;
if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel, true); + seclabel->imagelabel, false); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1701,7 +1701,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; break;
Are the EMULATOR changes actually required? I think it just uses a unix socket and doesn't touch the host kernel tpm subsystem - Cole
On 10/10/19 10:15 PM, Cole Robinson wrote:
On 10/1/19 11:00 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1755803
The /dev/tpmN file can be opened only once, as implemented in drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any other attempt to open the file fails. And since we're opening the file ourselves and passing the FD to qemu we will not succeed opening the file again when locking it for seclabel remembering.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 18 +++++++++--------- src/security/security_selinux.c | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2733fa664f..347a7a5f63 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1653,14 +1653,14 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.emulator.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.emulator.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1679,9 +1679,9 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACRestoreChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: /* swtpm will have removed the Unix socket upon termination */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e3be724a2b..0486bdd6b6 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1682,14 +1682,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel, true); + seclabel->imagelabel, false); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1701,7 +1701,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; break;
Are the EMULATOR changes actually required? I think it just uses a unix socket and doesn't touch the host kernel tpm subsystem
Well, not per se, because the socket is created on domain startup and then unlink()-ed on domain shutdown. That's why it doesn't make sense to try remember anything about the socket. Michal
On 10/11/19 11:08 AM, Michal Privoznik wrote:
On 10/10/19 10:15 PM, Cole Robinson wrote:
On 10/1/19 11:00 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1755803
The /dev/tpmN file can be opened only once, as implemented in drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any other attempt to open the file fails. And since we're opening the file ourselves and passing the FD to qemu we will not succeed opening the file again when locking it for seclabel remembering.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 18 +++++++++--------- src/security/security_selinux.c | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2733fa664f..347a7a5f63 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1653,14 +1653,14 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: - ret = virSecurityDACSetChardevLabel(mgr, def, - &tpm->data.emulator.source, - false); + ret = virSecurityDACSetChardevLabelHelper(mgr, def, + &tpm->data.emulator.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; @@ -1679,9 +1679,9 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - ret = virSecurityDACRestoreChardevLabel(mgr, def, - &tpm->data.passthrough.source, - false); + ret = virSecurityDACRestoreChardevLabelHelper(mgr, def, + &tpm->data.passthrough.source, + false, false); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: /* swtpm will have removed the Unix socket upon termination */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e3be724a2b..0486bdd6b6 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1682,14 +1682,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel, true); + seclabel->imagelabel, false); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1701,7 +1701,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, false); if (rc < 0) return -1; break;
Are the EMULATOR changes actually required? I think it just uses a unix socket and doesn't touch the host kernel tpm subsystem
Well, not per se, because the socket is created on domain startup and then unlink()-ed on domain shutdown. That's why it doesn't make sense to try remember anything about the socket.
Okay so it's basically a separate issue from the patch series. In that case use of virSecurityDACRestoreChardevLabelHelper for EMULATOR is a bit misleading, and instead virSecurityDACRestoreChardevLabel should probably grow some smarts to set remember=false for any unix socket path it sees. But it's a minor distinction because I don't think it matters in practice given that sockets are unlinked like you say Thanks, Cole
On 10/1/19 11:00 AM, Michal Privoznik wrote:
As it turns out, /dev/tpm0 can't be opened more than once. This doesn't fit into our seclabel remembering approach and thus disable it for TPM devices.
There's also another type of files which can't be opened more than once - /dev/vfio/N. Usually, this won't be a problem unless users try to attach/detach some devices from the same IOMMU group. This will require more treatment though because it's broken on more levels.
1) we remove /dev/vfio/N in private devtmpfs on device detach, even though there is another device still attached to domain from the same IOMMU group,
2) we remove the IOMMU group from CGroups, i.e. we effectively deny access to qemu
3) we restore seclabels (regardless of seclabel remembering)
Therefore, I'm only addressing TPM issue here and will continue work on hostdevs.
Michal Prívozník (3): security: Try to lock only paths with remember == true security_dac: Allow selective remember/recall for chardevs security: Don't remember labels for TPM
src/security/security_dac.c | 91 ++++++++++++++++++++++----------- src/security/security_selinux.c | 16 +++--- 2 files changed, 71 insertions(+), 36 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> but see comment on #3, I think the EMULATOR bits can be dropped. I verified this fixes TPM passthrough VM startup too - Cole
participants (2)
-
Cole Robinson -
Michal Privoznik