[libvirt] [PATCH 3/3] security: Don't remember labels for TPM
Cole Robinson
crobinso at redhat.com
Fri Oct 11 16:07:30 UTC 2019
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 at 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
More information about the libvir-list
mailing list