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

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>

> > + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { > > + VIR_ERROR("Could not start 'swtpm'. exitstatus: %d\n" > > + "stderr: %s\n", exitstatus, errbuf); > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("Could not start 'swtpm'. exitstatus: %d, " > > + "error: %s"), exitstatus, errbuf); > > + goto error; > > + } > > + > > + /* sync the startup of the swtpm's Unix socket with the start of QEMU */ > > + if (virTPMTryConnect(tpm->data.emulator.source.data.nix.path, > > + 3 * 1000 * 1000) < 0) { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("Could not connect to the swtpm on '%s'"), > > + tpm->data.emulator.source.data.nix.path); > > + goto error; > > + } > Ewww, this is really not nice to deal with startup races. > > You're using the --daemon flag to swtpm, so swtpm should make sure > that it doesn't daemonize itself & let the parent exit, until the > UNIX socket is actually ready to access connections. Can we just > get swtpm fixed in this way. It does that actually already and the socket file is there once it daemonizes itself.

> > + 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

So where do we put the swtpm's log files? /var/log/libvirt/swtpm?

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?

> > > + 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.