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>
> > > + 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.
If that's the case, why do you need this TryConnect busy waiting loop
at all ? It should be immediately available.
> > > + 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.
> So where do we put the swtpm's log files? /var/log/libvirt/swtpm?
Yeah, probably best to have a separate directory
> 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.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
