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
