2:29 p.m.
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(a)linux.vnet.ibm.com>
>> ---
>> 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