On Tue, May 15, 2018 at 05:50 PM +0200, Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
