9:49 a.m.
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@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@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