On 05/09/2018 01:47 PM, Stefan Berger wrote: > On 05/08/2018 04:30 PM, John Ferlan wrote: >> >> On 05/04/2018 04:21 PM, Stefan Berger wrote: >>> Add 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. >>> >>> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> >>> --- >>> src/libvirt_private.syms | 5 + >>> src/util/virtpm.c | 536 >>> ++++++++++++++++++++++++++++++++++++++++++++++- >>> src/util/virtpm.h | 33 ++- >>> 3 files changed, 572 insertions(+), 2 deletions(-) >>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>> index 33fe75b..eebfc72 100644 >>> --- a/src/libvirt_private.syms >>> +++ b/src/libvirt_private.syms >>> @@ -2984,6 +2984,11 @@ virTimeStringThenRaw; >>> # util/virtpm.h >>> virTPMCreateCancelPath; >>> +virTPMDeleteEmulatorStorage; >>> +virTPMEmulatorBuildCommand; >>> +virTPMEmulatorInitPaths; >>> +virTPMEmulatorPrepareHost; >>> +virTPMEmulatorStop; >>> # util/virtypedparam.h >>> diff --git a/src/util/virtpm.c b/src/util/virtpm.c >>> index d5c10da..76bbb21 100644 >>> --- a/src/util/virtpm.c >>> +++ b/src/util/virtpm.c >>> @@ -1,7 +1,7 @@ >>> /* >>> * virtpm.c: TPM support >>> * >>> - * Copyright (C) 2013 IBM Corporation >>> + * Copyright (C) 2013,2018 IBM Corporation >>> * >>> * This library is free software; you can redistribute it and/or >>> * modify it under the terms of the GNU Lesser General Public >>> @@ -22,16 +22,36 @@ >>> #include <config.h> >>> +#include <sys/types.h> >>> #include <sys/stat.h> >>> +#include <unistd.h> >>> +#include <fcntl.h> >>> +#include <cap-ng.h> >>> +#include "conf/domain_conf.h" >> syntax-check would have told you unsafe cross-directory include - IOW >> including conf/* files into util/* files is not allowed. >> >> So I think you need to rethink where some of these functions will go. I >> think they are mostly all used by the qemu_extdevice.c changes in patch >> 9, so perhaps they need to get folded into them. There at least you can >> grab the conf/domain_conf.h file. > > Probably best to do that... rather than passing the fields of > virDomainTPMDef into the functions instead. > Currently the functions have the prefix virTPM. That will have to change > - to qemuTPM? So I'll merge these functions into qemu_extdevice.c? or > another new file qemu_tpm.c ? > > qemu_tpm.c seems good for those specific things > >> >>> +#include "viralloc.h" >> syntax-check would have told you not to include "viralloc.h" twice... > > obviously 'forgot' to run it. > >> >>> +#include "vircommand.h" >>> #include "virstring.h" >>> #include "virerror.h" >>> #include "viralloc.h" >>> #include "virfile.h" >>> +#include "virkmod.h" >>> +#include "virlog.h" >>> #include "virtpm.h" >>> +#include "virutil.h" >> #include "viruuid.h" to get virUUIDGenerate >> >>> +#include "configmake.h" >>> #define VIR_FROM_THIS VIR_FROM_NONE >>> +VIR_LOG_INIT("util.tpm") >>> + >>> +/* >>> + * executables for the swtpm; to be found on the host >>> + */ >>> +static char *swtpm_path; >>> +static char *swtpm_setup; >>> +static char *swtpm_ioctl; >>> + >> There's a love/hate relationship with static/globals... >> >>> /** >>> * virTPMCreateCancelPath: >>> * @devpath: Path to the TPM device >>> @@ -74,3 +94,517 @@ virTPMCreateCancelPath(const char *devpath) >>> cleanup: >>> return path; >>> } >> Two empty lines - pervasive comment here... >> >>> + >>> +/* >>> + * virTPMEmulatorInit >>> + * >>> + * Initialize the Emulator functions by searching for necessary >>> + * executables that we will use to start and setup the swtpm >>> + */ >>> +static int >>> +virTPMEmulatorInit(void) >>> +{ >>> + if (!swtpm_path) { >>> + swtpm_path = virFindFileInPath("swtpm"); >>> + if (!swtpm_path) { >>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >>> + _("Could not find swtpm 'swtpm' in PATH")); >> The message feels redundant. > > You mean the repetition of 'swtpm' is redundant? yes. > > Should I adapt the reporting to use this type of command ? > > if (!(qemunbd = virFindFileInPath("qemu-nbd"))) { > virReportSystemError(ENOENT, "%s", > _("Unable to find 'qemu-nbd' binary in > $PATH")); > goto cleanup; > } > + That seems reasonable. >>> +/* >>> + * virTPMEmulatorPrepareHost: >>> + * >>> + * @tpm: tpm definition >>> + * @logDir: directory where swtpm writes its logs into >>> + * @vmname: name of the VM >>> + * @swtpm_user: uid to run the swtpm with >>> + * @swtpm_group: gid to run the swtpm with >>> + * @swtpmStateDir: directory for swtpm's persistent state >>> + * @qemu_user: uid that qemu will run with; we share the socket file >>> with it >>> + * @shortName: short and unique name of the domain >>> + * >>> + * Prepare the log directory for the swtpm and adjust ownership of >>> it and the >>> + * log file we will be using. Prepare the state directory where we >>> will share >>> + * the socket between tss and qemu users. >>> + */ >>> +int virTPMEmulatorPrepareHost(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) >> one line each argument >> >>> +{ >>> + int ret = -1; >>> + >>> + if (virTPMEmulatorInit() < 0) >>> + return -1; >>> + >>> + /* create log dir ... */ >>> + if (virFileMakePathWithMode(logDir, 0730) < 0) >>> + goto cleanup; >> I think we could just return -1; here. >> >>> + >>> + /* ... and adjust ownership */ >>> + if (virDirCreate(logDir, 0730, swtpm_user, swtpm_group, >>> + VIR_DIR_CREATE_ALLOW_EXIST) < 0) >>> + goto cleanup; >>> + >>> + /* create logfile name ... */ >>> + if (virAsprintf(&tpm->data.emulator.logfile, "%s/%s-swtpm.log", >>> + logDir, vmname) < 0) >>> + 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 = >>> + virTPMCreateEmulatorSocket(swtpmStateDir, shortName))) >>> + goto cleanup; >>> + tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; >>> + >>> + ret = 0; >>> + >>> + cleanup: >>> + if (ret) >>> + VIR_FREE(tpm->data.emulator.logfile); >> Does this matter? Wouldn't the logfile be free'd in a failure path by >> virDomainTPMDefFree? If it does matter, use "if (ret < 0)". > > I'll remove that but add a check for tpm->data.emulator.logfile before > 'virAsprintf(&tpm->data.emulator.logfile,', so we don't have a memory > leak here. > ah - oh right - that's probably better anyway. >> >>> + >>> + return ret; >>> +} >>> + >>> +/* >>> + * virTPMEmulatorRunSetup >>> + * >>> + * @storagepath: path to the directory for TPM state >>> + * @vmname: the name of the VM >>> + * @vmuuid: the UUID of the VM >>> + * @privileged: whether we are running in privileged mode >>> + * @swtpm_user: The userid to switch to when setting up the TPM; >>> + * typically this should be the uid of 'tss' or 'root' >>> + * @swtpm_group: The group id to switch to >>> + * @logfile: The file to write the log into; it must be writable >>> + * for the user given by userid or 'tss' >>> + * >>> + * Setup the external swtpm by creating endorsement key and >>> + * certificates for it. >>> + */ >>> +static int >>> +virTPMEmulatorRunSetup(const char *storagepath, const char *vmname, >>> + const unsigned char *vmuuid, bool privileged, >>> + uid_t swtpm_user, gid_t swtpm_group, >>> + const char *logfile) >> One arg per line >> >>> +{ >>> + virCommandPtr cmd = NULL; >>> + int exitstatus; >>> + int rc = 0; >> Use ret = -1; >> >>> + char uuid[VIR_UUID_STRING_BUFLEN]; >>> + char *vmid = NULL; >>> + off_t logstart; >>> + >>> + if (!privileged) { >>> + return virFileWriteStr(logfile, >>> + _("Did not create EK and certificates >>> since " >>> + "this requires privileged mode\n"), >>> + 0600); >>> + } >>> + >>> + cmd = virCommandNew(swtpm_setup); >>> + if (!cmd) { >>> + rc = -1; >>> + goto cleanup; >>> + } >> Just goto cleanup; w/ @ret initialized to -1 >> >>> + >>> + virUUIDFormat(vmuuid, uuid); >>> + if (virAsprintf(&vmid, "%s:%s", vmname, uuid) < 0) >>> + goto cleanup; >> Because here you would have returned 0 and I don't think that's what >> you'd expect at this point... >> >>> + >>> + virCommandSetUID(cmd, swtpm_user); >>> + virCommandSetGID(cmd, swtpm_group); >>> + >>> + virCommandAddArgList(cmd, >>> + "--tpm-state", storagepath, >>> + "--vmid", vmid, >>> + "--logfile", logfile, >>> + "--createek", >>> + "--create-ek-cert", >>> + "--create-platform-cert", >>> + "--lock-nvram", >>> + "--not-overwrite", >>> + NULL); >>> + >>> + virCommandClearCaps(cmd); >>> + >>> + /* get size of logfile */ >>> + logstart = virFileLength(logfile, -1); >>> + if (logstart < 0) >>> + logstart = 0; >>> + >>> + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { >>> + char *buffer = NULL, *errors; >>> + off_t loglength = virFileLength(logfile, -1); >>> + >>> + if (loglength > logstart) { >>> + ignore_value(virFileReadOffsetQuiet(logfile, logstart, >>> + loglength - logstart, >>> + &buffer)); >> On error @buffer could be NULL >> >>> + errors = virStringFilterLines(buffer, "Error:", 160); >> If @buffer == NULL, then the above is not going to work. >> >> Also should it be _("Error:") or are we sure that the program is run >> with a specific language (e.g. i18n concerns)? Since we don't control >> the language of that output - it's a bit dicey to assume/use it. >> >>> + virReportError(VIR_ERR_INTERNAL_ERROR, >>> + _("Could not run '%s'. exitstatus: %d;\n" >>> + "%s"), >>> + swtpm_setup, exitstatus, errors); >> Given the "concerns" raised, why not just direct the consumer to the >> @logfile rather than trying to report the error into the output. > > I had that before and thought it was more convenient to show to the user > what the problem was. > >> >> >> IOW: >> >> if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { >> virReportError(VIR_ERR_INTERNAL_ERROR, >> _("Could not run '%s'. exitstatus: %d;\n" >> "Check error log '%s' for details."), >> swtpm_setup, exitstatus, logfile); >> goto cleanup; >> } >> >> >> If you really want to keep this logic, then handling buffer == NULL >> before calling virStringFilterLines will need to be done and of course >> handling that @errors wouldn't be filled in. > > I am not insisting ... let me drop this along with patches 1 & 2. > At some point the handling of errors in an error path just becomes one of those seems like a good idea. I didn't mind the extra lines of output and effort because I'm one who likes verbose error messages. John >> >>> + VIR_FREE(buffer); >>> + VIR_FREE(errors); >>> + } >>> + rc = -1; >> goto cleanup; >> >>> + } >>> + >> ret = 0; >> >>> + cleanup: >>> + VIR_FREE(vmid); >>> + virCommandFree(cmd); >>> + >>> + return rc; >> return ret; >> >>> +} >>> + >>> +/* >>> + * virTPMEmulatorBuildCommand: >>> + * >>> + * @tpm: TPM definition >>> + * @vmname: The name of the VM >>> + * @vmuuid: The UUID of the VM >>> + * @privileged: whether we are running in privileged mode >>> + * @swtpm_user: The uid for the swtpm to run as (drop privileges to >>> from root) >>> + * @swtpm_group: The gid for the swtpm to run as >>> + * >>> + * Create the virCommand use for starting the emulator >>> + * Do some initializations on the way, such as creation of storage >>> + * and emulator setup. >>> + */ >>> +virCommandPtr >>> +virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, >>> + const unsigned char *vmuuid, bool >>> privileged, >>> + uid_t swtpm_user, gid_t swtpm_group) >> One arg per line >> >>> +{ >>> + virCommandPtr cmd = NULL; >>> + bool created = false; >>> + >>> + if (virTPMCreateEmulatorStorage(tpm->data.emulator.storagepath, >>> + &created, swtpm_user, >>> swtpm_group) < 0) >>> + return NULL; >>> + >>> + if (created && >>> + virTPMEmulatorRunSetup(tpm->data.emulator.storagepath, >>> vmname, vmuuid, >>> + privileged, swtpm_user, swtpm_group, >>> + tpm->data.emulator.logfile) < 0) >>> + goto error; >>> + >>> + unlink(tpm->data.emulator.source.data.nix.path); >>> + >>> + 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,mode=0600", >>> + tpm->data.emulator.storagepath); >>> + >>> + virCommandAddArg(cmd, "--log"); >>> + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile); >>> + >>> + virCommandSetUID(cmd, swtpm_user); >>> + virCommandSetGID(cmd, swtpm_group); >>> + >>> + return cmd; >>> + >>> + error: >>> + if (created) >>> + virTPMDeleteEmulatorStorage(tpm); >>> + >>> + VIR_FREE(tpm->data.emulator.source.data.nix.path); >>> + VIR_FREE(tpm->data.emulator.storagepath); >> Similar question here about the VIR_FREE's - why not wait for >> virDomainTPMDefFree? > > Dropped these as well. > > >> >>> + >>> + virCommandFree(cmd); >>> + >>> + return NULL; >>> +} >>> + >>> +/* >>> + * virTPMEmulatorStop >>> + * @swtpmStateDir: A directory where the socket is located >>> + * @shortName: short and unique name of the domain >>> + * >>> + * Gracefully stop the swptm >>> + */ >>> +void >>> +virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) >>> +{ >>> + virCommandPtr cmd; >>> + char *pathname; >>> + char *errbuf = NULL; >>> + >>> + if (virTPMEmulatorInit() < 0) >>> + return; >>> + >>> + if (!(pathname = virTPMCreateEmulatorSocket(swtpmStateDir, >>> shortName))) >>> + return; >>> + >>> + if (!virFileExists(pathname)) >>> + goto cleanup; >>> + >>> + cmd = virCommandNew(swtpm_ioctl); >>> + if (!cmd) { >>> + VIR_FREE(pathname); >> ^^^^ >> Done in cleanup anyway. >> >>> + goto cleanup; >>> + } >>> + >>> + virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL); >>> + >>> + virCommandSetErrorBuffer(cmd, &errbuf); >>> + >>> + ignore_value(virCommandRun(cmd, NULL)); >>> + >>> + virCommandFree(cmd); >>> + >>> + /* clean up the socket */ >>> + unlink(pathname); >>> + >>> + cleanup: >>> + VIR_FREE(pathname); >>> + VIR_FREE(errbuf); >>> +} >>> diff --git a/src/util/virtpm.h b/src/util/virtpm.h >>> index b21fc05..63f75b8 100644 >>> --- a/src/util/virtpm.h >>> +++ b/src/util/virtpm.h >>> @@ -1,7 +1,7 @@ >>> /* >>> * virtpm.h: TPM support >>> * >>> - * Copyright (C) 2013 IBM Corporation >>> + * Copyright (C) 2013,2018 IBM Corporation >>> * >>> * This library is free software; you can redistribute it and/or >>> * modify it under the terms of the GNU Lesser General Public >>> @@ -22,6 +22,37 @@ >>> #ifndef __VIR_TPM_H__ >>> # define __VIR_TPM_H__ >>> +# include "vircommand.h" >>> + >>> +typedef struct _virDomainTPMDef virDomainTPMDef; >>> +typedef virDomainTPMDef *virDomainTPMDefPtr; >>> + >> Duplicated from domain_conf.h to avoid errors, crafty... >> >> >> I have a feeling most of this is going to be merged into patch 9... > > Thanks for reviewing. > > Stefan > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list