[libvirt] [PATCH v4 09/11] security: Label the external swtpm with SELinux labels
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue May 15 17:21:35 UTC 2018
On 05/15/2018 12:13 PM, Marc Hartmayer wrote:
> On Tue, May 15, 2018 at 05:50 PM +0200, Stefan Berger <stefanb at 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 at 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 at 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 at 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 at 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 at 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 at 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
More information about the libvir-list
mailing list