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

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> > > The XML will currently only start a TPM 1.2. > > Upon the first start, libvirt will run `swtpm_setup`, which will simulate the > manufacturing of a TPM and create certificates for it and write them into the > NVRAM location of the emulated TPM. > > Then, libvirt will automatically start the swtpm TPM emulator using the `swtpm` > executable. > > Once the VM terminates, libvirt uses the swtpm_ioctl executable to gracefully > shut down the `swtpm` in case it is still running (QEMU did not send shutdown) > or clean up the socket file. > > The above mentioned executables must be found in the PATH. > > The executables can either be run as root or started as root and switch to > the tss user. The requirement for the tss user comes through 'tcsd', which > is used for the simulation of the manufacturing. Which user is used can be > configured through qemu.conf. > > The swtpm writes out state into files. The state is kept in /var/lib/libvirt/tpm: > > [root@localhost libvirt]# ls -lZ | grep tpm > > drwx--x--x. 7 root root unconfined_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:22 tpm > > The directory /var/lib/libvirt/tpm maintains per-TPM state directories but > also hosts the UnixIO socket of running swtpms, which QEMU uses for communicating > with them. At this point only the socket file is labeled properly and made accessible > for QEMU, which runs under the qemu user: /var/lib is for persistent state while /var/run is for transient state, so I think sockets should be under /var/run instead.

> [root@localhost tpm]# ls -lZ > total 4 > drwx------. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:46 485d0004-a48f-436a-8457-8a3b73e28567 > srw-------. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c413,c430 0 Apr 5 16:46 485d0004-a48f-436a-8457-8a3b73e28567.sock > > [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ls -lZ > total 8 > -rw-r--r--. 1 tss tss system_u:object_r:virt_var_lib_t:s0 3648 Apr 5 16:46 tpm-00.permall > -rw-r--r--. 1 tss tss system_u:object_r:virt_var_lib_t:s0 2237 Apr 5 16:46 vtpm.log > > root@sbct-3 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep -v grep > system_u:system_r:virtd_t:s0-s0:c0.c1023 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/lib/libvirt/tpm/485d0004-a48f-436a-8457-8a3b73e28567.sock,mode=0600 --tpmstate dir=/var/lib/libvirt/tpm/485d0004-a48f-436a-8457-8a3b73e28567 --log file=/var/lib/libvirt/tpm/485d0004-a48f-436a-8457-8a3b73e28567/vtpm.log --runas 59 > > [root@sbct-3 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep > system_u:system_r:svirt_t:s0:c413,c430 qemu 18702 2.5 0.0 3036052 48676 ? Sl 16:46 0:08 /bin/qemu-system-x86_64 -name guest=centos7.0,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-6-centos7.0/master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-core=off -cpu kvm64 -m 2048 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid 485d0004-a48f-436a-8457-8a3b73e28567 [...] -tpmdev emulator,id=tpm-tpm0,chardev=chrtpm -chardev socket,id=chrtpm,path=/var/lib/libvirt/tpm/485d0004-a48f-436a-8457-8a3b73e28567.sock -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0 -device usb-mouse,id=input0,bus=usb.0,port=1 -vnc 127.0.0.1:0 -device cirrus-vga,id=video0,bus=pci.0,addr=0x2 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6 -msg timestamp=on > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > docs/formatdomain.html.in | 30 ++ > docs/schemas/domaincommon.rng | 5 + > src/conf/domain_audit.c | 2 + > src/conf/domain_conf.c | 51 ++- > src/conf/domain_conf.h | 5 + > src/libvirt_private.syms | 5 + > src/qemu/Makefile.inc.am | 2 + > src/qemu/libvirtd_qemu.aug | 3 + > src/qemu/qemu.conf | 7 + > src/qemu/qemu_capabilities.c | 5 + > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_cgroup.c | 1 + > src/qemu/qemu_command.c | 52 ++- > src/qemu/qemu_conf.c | 11 +- > src/qemu/qemu_conf.h | 2 + > src/qemu/qemu_domain.c | 2 + > src/qemu/qemu_driver.c | 13 + > src/qemu/qemu_extdevice.c | 195 ++++++++++ > src/qemu/qemu_extdevice.h | 36 ++ > src/qemu/qemu_process.c | 8 + > src/qemu/test_libvirtd_qemu.aug.in | 1 + > src/security/security_dac.c | 6 + > src/security/security_selinux.c | 11 + > src/util/virfile.c | 12 + > src/util/virfile.h | 2 +- > src/util/virtpm.c | 432 +++++++++++++++++++++ > src/util/virtpm.h | 12 + > tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + > tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + > tests/qemuxml2argvdata/tpm-emulator.args | 24 ++ > tests/qemuxml2argvdata/tpm-emulator.xml | 30 ++ > tests/qemuxml2argvmock.c | 2 + > tests/qemuxml2argvtest.c | 17 + > tests/qemuxml2xmloutdata/tpm-emulator.xml | 34 ++ > tests/qemuxml2xmltest.c | 1 + > 38 files changed, 1011 insertions(+), 14 deletions(-) > create mode 100644 src/qemu/qemu_extdevice.c > create mode 100644 src/qemu/qemu_extdevice.h > create mode 100644 tests/qemuxml2argvdata/tpm-emulator.args > create mode 100644 tests/qemuxml2argvdata/tpm-emulator.xml > create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator.xml > +/* > + * qemuExtTPMStartEmulator: > + * > + * @comm: virConnect pointer > + * @driver: QEMU driver > + * @vm: domain object > + * > + * Start the external TPM Emulator: > + * - have the command line built > + * - start the external TPM Emulator and sync with it before QEMU start > + */ > +static int > +qemuExtTPMStartEmulator(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + qemuDomainLogContextPtr logCtxt) > +{ > + int ret = -1; > + virCommandPtr cmd = NULL; > + int exitstatus; > + char *errbuf = NULL; > + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > + virDomainDefPtr def = vm->def; > + unsigned char *vmuuid = def->uuid; > + virDomainTPMDefPtr tpm = def->tpm; > + > + /* stop any left-over TPM emulator for this VM */ > + virTPMStopEmulator(tpm, vmuuid, false); > + > + if (!(cmd = virTPMEmulatorBuildCommand(tpm, vmuuid, cfg->swtpm_user))) > + goto cleanup; > + > + if (qemuExtDeviceLogCommand(logCtxt, cmd, "TPM Emulator") < 0) > + goto cleanup; > + > + virCommandSetErrorBuffer(cmd, &errbuf); > + > + 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.

> +/* > + * virTPMSetupEmulator > + * > + * @storagepath: path to the directory for TPM state > + * @vmuuid: the UUID of the VM > + * @userid: The userid to switch to when setting up the TPM; > + * typically this should be 'tss' > + * @logfile: The file to write the log into; it must be writable > + * for the user given by userid or 'tss' > + * > + * Setup the external swtpm > + */ > +static int > +virTPMSetupEmulator(const char *storagepath, const unsigned char *vmuuid, > + uid_t swtpm_user, const char *logfile) > +{ > + virCommandPtr cmd = NULL; > + int exitstatus; > + int rc = 0; > + char uuid[VIR_UUID_STRING_BUFLEN]; > + > + cmd = virCommandNew(swtpm_setup); > + if (!cmd) { > + rc = -1; > + goto cleanup; > + } > + > + virUUIDFormat(vmuuid, uuid); > + > + if (swtpm_user > 0) { > + virCommandAddArg(cmd, "--runas"); > + virCommandAddArgFormat(cmd, "%u", swtpm_user); > + } > + virCommandAddArgList(cmd, > + "--tpm-state", storagepath, > + "--vmid", uuid, > + "--logfile", logfile, > + "--createek", > + "--create-ek-cert", > + "--create-platform-cert", > + "--lock-nvram", > + "--not-overwrite", > + NULL); > + > + virCommandClearCaps(cmd); > + > + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { > + /* copy the log to libvirt error since the log will be deleted */ > + char *buffer = NULL; > + ignore_value(virFileReadAllQuiet(logfile, 10240, &buffer)); > + VIR_ERROR(_("Error setting up swtpm:\n%s"), buffer); This is not nice - VIR_ERROR should never be used to report problems that occurr during guest startup - it needs to be in the error message reported... > + VIR_FREE(buffer); > + > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not run '%s'. exitstatus: %d; " > + "please check the libvirt error log"), > + swtpm_setup, exitstatus); ....here.

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

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

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

If we're starting one swtpm per QEMU, we should also make sure it gets put into the cgroup associated with that QEMU

> + > + /* clean up the socket */ > + unlink(pathname); > + VIR_FREE(pathname); > + > + VIR_FREE(tpm->data.emulator.source.data.nix.path); > + tpm->data.emulator.source.type = 0; > + VIR_FREE(errbuf); > +} Regards, Daniel