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

[Daniel P. Berrangé]

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

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