[libvirt] [PATCH 2/6] tpm: Add support for external swtpm TPM emulator

[Stefan Berger]

On 04/06/2018 10:12 AM, Daniel P. Berrangé wrote:
> On Fri, Apr 06, 2018 at 07:23:49AM -0400, Stefan Berger wrote:
>> On 04/06/2018 04:26 AM, Daniel P. Berrangé wrote:
>>> On Thu, Apr 05, 2018 at 05:56:02PM -0400, Stefan Berger wrote:
>>>> This patch adds support for an external swtpm TPM emulator. The XML for
>>>> this type of TPM looks as follows:
>>>>
>>>>    <tpm model='tpm-tis'>
>>>>      <backend type='emulator'/>
>>>>    </tpm>
>

>
>
>>>> +    cmd = virCommandNew(swtpm_path);
>>>> +    if (!cmd)
>>>> +        goto error;
>>>> +
>>>> +    virCommandClearCaps(cmd);
>>>> +
>>>> +    virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL);
>>>> +    virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600",
>>>> +                           tpm->data.emulator.source.data.nix.path);
>>>> +
>>>> +    virCommandAddArg(cmd, "--tpmstate");
>>>> +    virCommandAddArgFormat(cmd, "dir=%s", storagepath);
>>>> +
>>>> +    virCommandAddArg(cmd, "--log");
>>>> +    virCommandAddArgFormat(cmd, "file=%s", logfile);
>>>> +
>>>> +    /* allow process to open logfile by root before dropping privileges */
>>>> +    virCommandAllowCap(cmd, CAP_DAC_OVERRIDE);
>>> Why can't we get have the log file be owned by the user that
>>> swtpm runs as, instead of root ?
>> I would have to look at this particular capability again. I initially wanted
>> to put the swtpm's log file also into /var/log/libvirt/qemu. It works nice
>> of course when running swtpm as 'root' but not so much when running it as
>> 'tss':
>>
>> root at localhost tmp]$ sudo ls -l /var/log/libvirt/ | grep qemu
>> drwx------. 2 root root 20480 Apr  5 16:14 qemu
> Yeah the logs are owned by root these days, because they're not written by
> qemu itself, instead virtlogd owns it.

[root at localhost log]# ls -lZ | grep libvirt
drwx------. 6 root      root 
system_u:object_r:virt_log_t:s0                   4096 Mar  1  2017 libvirt

Even /var/log/libvirt would not be accessible for the tss users.

>
>> So where do we put the swtpm's log files? /var/log/libvirt/swtpm?
> Yeah, probably best to have a separate directory

It would have to be /var/log/swtpm unless we change the permissions on 
/var/log/libvirt ... ?

>> Iirc the CAP_DAC_OVERRIDE became necessary when swtpm tries to append to the
>> log that 'tss' owns but now libvirt runs it as 'root'. I think that was the
>> reason I added it. One way to solve this would be to chown() the files
>> before starting swtpm. Is that the solution?
> I'd think a separate dir is best
>
>>
>>>> +    if (swtpm_user > 0) {
>>>> +        virCommandAddArg(cmd, "--runas");
>>>> +        virCommandAddArgFormat(cmd, "%u", swtpm_user);
>>>> +        virCommandAllowCap(cmd, CAP_SETGID);
>>>> +        virCommandAllowCap(cmd, CAP_SETUID);
>>>> +    }
>>> Then we could tell virCommand to set the UID/GID before it even
>>> executes this, and not use -runas, thus avoiding the need to
>>> allow theses capabilities.
>> And the directory it writes the logs in would have to have ownership of
>> either root:root or tss:root (or tss:tss), depending on how libvirt is
>> currently configured? Again chown() the dir before starting?
>>
>>>
>>>> +/*
>>>> + * virTPMStopEmulator
>>>> + * @tpm: TPM definition
>>>> + * @vmuuid: the UUID of the VM
>>>> + * @verbose: whether to report errors
>>>> + *
>>>> + * Gracefully stop the swptm
>>>> + */
>>>> +void
>>>> +virTPMStopEmulator(virDomainTPMDefPtr tpm, const unsigned char *vmuuid,
>>>> +                   bool verbose)
>>>> +{
>>>> +    virCommandPtr cmd;
>>>> +    int exitstatus;
>>>> +    char *pathname;
>>>> +    char *errbuf = NULL;
>>>> +
>>>> +    (void)vmuuid;
>>>> +    if (virTPMEmulatorInit() < 0)
>>>> +        return;
>>>> +
>>>> +    if (!(pathname = virTPMCreateEmulatorSocket(vmuuid)))
>>>> +        return;
>>>> +
>>>> +    cmd = virCommandNew(swtpm_ioctl);
>>>> +    if (!cmd) {
>>>> +        VIR_FREE(pathname);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL);
>>>> +
>>>> +    virCommandSetErrorBuffer(cmd, &errbuf);
>>>> +
>>>> +    if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
>>>> +        if (verbose)
>>>> +            VIR_ERROR(_("Could not run swtpm_ioctl -s '%s'."
>>>> +                      " existstatus: %d\nstderr: %s"),
>>>> +                      swtpm_ioctl, exitstatus, errbuf);
>>>> +    }
>>>> +
>>>> +    virCommandFree(cmd);
>>> I would feel better if we just directly killed the process - with
>>> this approach if something goes wrong with swtpm it may never
>>> respond to this request and stay running.
>> swtpm can write a pidfile. I am only adding this later in this series.
>> Problem is with --daemon libvirt doesn't know the pid of the swtpm anymore.
> The other option is to not use --daemon, and let libvirt write the pid
> file, but that introduces the race with socket path creation again
> which is not good.

Sounds like we should leave this as it is? Unless swtpm was broken, 
there shouldn't be a reason why the we wouldn't be able to shut down 
swtpm by sending a command to it. The socket and its directory must not 
have disappeared of course.

   Stefan

>
>
> Regards,
> Daniel