1:59 a.m.
On 05/23/2018 11:41 AM, Ján Tomko wrote:
On Tue, May 22, 2018 at 04:44:47PM -0400, Stefan Berger wrote:
> Implement functions for managing the storage of the external swtpm as
> well
> as starting and stopping it. Also implement functions to use
> swtpm_setup,
> which simulates the manufacturing of a TPM, which includes creation of
> certificates for the device.
>
> Further, the external TPM needs storage on the host that we need to set
> up before it can be run. We can clean up the host once the domain is
> undefined.
>
> This patch also implements a small layer for external device support
> that
> calls into the TPM device layer if a domain has an attached TPM. This is
> the layer we will wire up later on.
>
Later on meaning in other patch series? Adding it for just one device
seems excessive, but that might be my personal opinion.
> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/Makefile.inc.am | 4 +
> src/qemu/qemu_domain.c | 2 +
> src/qemu/qemu_extdevice.c | 154 ++++++++++
> src/qemu/qemu_extdevice.h | 53 ++++
> src/qemu/qemu_process.c | 12 +
> src/qemu/qemu_tpm.c | 751
> ++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_tpm.h | 50 +++
> 7 files changed, 1026 insertions(+)
> create mode 100644 src/qemu/qemu_extdevice.c
> create mode 100644 src/qemu/qemu_extdevice.h
> create mode 100644 src/qemu/qemu_tpm.c
> create mode 100644 src/qemu/qemu_tpm.h
>
> +/*
> + * virtTPMGetTPMStorageDir:
> + *
> + * @storagepath: directory for swtpm's persistent state
> + *
> + * Derive the 'TPMStorageDir' from the storagepath by searching
> + * for the last '/'.
> + */
> +static char *
> +qemuTPMGetTPMStorageDir(const char *storagepath)
> +{
> + const char *tail = strrchr(storagepath, '/');
> + char *path = NULL;
> +
> + if (!tail) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get tail of storagedir %s"),
> + storagepath);
> + return NULL;
> + }
> + ignore_value(VIR_STRNDUP(path, storagepath, tail - storagepath));
> +
> + return path;
> +}
In other places we already use mdir_name from gnulib for this
functionality.
I didn't know about this API. I will change it to that.
> +/*
> + * qemuTPMCreateEmulatorStorage
> + *
> + * @storagepath: directory for swtpm's persistent state
> + * @created: a pointer to a bool that will be set to true if the
> + * storage was created because it did not exist yet
Can't the caller call virFileExists and act accordingly?
We could, except that we have to call this function since it is
adjusting access rights to files for the swtpm_user and swtpm_group.
> + * @swtpm_user: The uid that needs to be able to access the directory
> + * @swtpm_group: The gid that needs to be able to access the directory
> + *
> + * Unless the storage path for the swtpm for the given VM
> + * already exists, create it and make it accessible for the given
> userid.
> + * Adapt ownership of the directory and all swtpm's state files there.
> + */
[...]
> +static int
> +qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
> + const char *logDir,
> + const char *vmname,
> + uid_t swtpm_user,
> + gid_t swtpm_group,
> + const char *swtpmStateDir,
> + uid_t qemu_user,
> + const char *shortName)
> +{
> + int ret = -1;
> +
> + if (qemuTPMEmulatorInit() < 0)
> + return -1;
> +
> + /* create log dir ... allow 'tss' user to cd into it */
> + if (virFileMakePathWithMode(logDir, 0711) < 0)
> + return -1;
> +
> + /* ... and adjust ownership */
> + if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group,
> + VIR_DIR_CREATE_ALLOW_EXIST) < 0)
> + goto cleanup;
> +
> + /* create logfile name ... */
> + if (!tpm->data.emulator.logfile &&
> + virAsprintf(&tpm->data.emulator.logfile,
"%s/%s-swtpm.log",
> + logDir, vmname) < 0)
This should also use shortName.
The shortName has the ID of the domain in the name. So for short-lived
logs I would say yes. Though this should be a log like the one for the
VM that gets appended to every time the VM restarts. I'd rather not
change this.
> + goto cleanup;
> +
> + /* ... and make sure it can be accessed by swtpm_user */
> + if (virFileExists(tpm->data.emulator.logfile) &&
> + chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) <
> 0) {
> + virReportSystemError(errno,
> + _("Could not chown on swtpm logfile %s"),
> + tpm->data.emulator.logfile);
> + goto cleanup;
> + }
> +
> + /*
> + create our swtpm state dir ...
> + - QEMU user needs to be able to access the socket there
> + - swtpm group needs to be able to create files there
> + - in privileged mode 0570 would be enough, for non-privileged
> mode
> + we need 0770
> + */
> + if (virDirCreate(swtpmStateDir, 0770, qemu_user, swtpm_group,
> + VIR_DIR_CREATE_ALLOW_EXIST) < 0)
> + goto cleanup;
> +
> + /* create the socket filename */
> + if (!tpm->data.emulator.source.data.nix.path &&
> + !(tpm->data.emulator.source.data.nix.path =
> + qemuTPMCreateEmulatorSocket(swtpmStateDir, shortName)))
> + goto cleanup;
> + tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX;
> +
> + ret = 0;
> +
> + cleanup:
> +
> + return ret;
> +}
> +
[...]
> +static int
> +qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + qemuDomainLogContextPtr logCtxt)
> +{
> + int ret = -1;
> + virCommandPtr cmd = NULL;
> + int exitstatus;
> + char *errbuf = NULL;
> + virQEMUDriverConfigPtr cfg;
> + virDomainTPMDefPtr tpm = def->tpm;
> + char *shortName = virDomainDefGetShortName(def);
> +
> + if (!shortName)
> + return -1;
> +
> + cfg = virQEMUDriverGetConfig(driver);
> +
> + /* stop any left-over TPM emulator for this VM */
> + qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
> +
> + if (!(cmd = qemuTPMEmulatorBuildCommand(tpm, def->name, def->uuid,
> + driver->privileged,
> + cfg->swtpm_user,
> + cfg->swtpm_group)))
> + goto cleanup;
> +
> + if (qemuExtDeviceLogCommand(logCtxt, cmd, "TPM Emulator") < 0)
> + goto cleanup;
> +
> + virCommandSetErrorBuffer(cmd, &errbuf);
> +
> + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not start 'swtpm'. exitstatus: %d,
"
> + "error: %s"), exitstatus, errbuf);
Indentation is off here ^
Fixed.
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
Jano
Thanks..
Stefan