10 a.m.
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
10 a.m.
New subject: [libvirt] [PATCH 1/3] security: Try to lock only paths with remember == true
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(a)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
10 a.m.
New subject: [libvirt] [PATCH 2/3] security_dac: Allow selective remember/recall for chardevs
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(a)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
10 a.m.
New subject: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM
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(a)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
3:15 p.m.
New subject: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM
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(a)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
10:08 a.m.
New subject: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM
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(a)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
11:07 a.m.
New subject: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM
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(a)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
3:16 p.m.
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(a)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
1905
days inactive
1915
days old
7 comments
2 participants
participants (2)
-
Cole Robinson -
Michal Privoznik