Re: [Qemu-devel] [PATCH v2 9/9] tpm: Added support for TPM emulator

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 04/10/2017 03:08 AM, Amarnath Valluri wrote:
>
>
> On 07.04.2017 17:41, Daniel P. Berrange wrote:
>> On Fri, Apr 07, 2017 at 05:30:31PM +0300, Amarnath Valluri wrote:
>>> This change introduces a new TPM backend driver that can communicate 
>>> with
>>> swtpm(software TPM emulator) using unix domain socket interface.
>>>
>>> Swtpm uses two unix sockets, one for plain TPM commands and 
>>> responses, and one
>>> for out-of-band control messages.
>>>
>>> The swtpm and associated tools can be found here:
>>>      https://github.com/stefanberger/swtpm
>>>
>>> Usage:
>>>      # setup TPM state directory
>>>      mkdir /tmp/mytpm
>>>      chown -R tss:root /tmp/mytpm
>>>      /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek
>>>
>>>      # Ask qemu to use TPM emulator with given tpm state directory
>>>      qemu-system-x86_64 \
>>>          [...] \
>>>          -tpmdev 
>>> emulator,id=tpm0,tpmstatedir=/tmp/mytpm,logfile=/tmp/swtpm.log \
>>>          -device tpm-tis,tpmdev=tpm0 \
>>>          [...]
>>>
>>> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
>>> ---
>>>   configure             |  15 +-
>>>   hmp.c                 |  21 ++
>>>   hw/tpm/Makefile.objs  |   1 +
>>>   hw/tpm/tpm_emulator.c | 927 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/tpm/tpm_ioctl.h    | 243 +++++++++++++
>>>   qapi-schema.json      |  36 +-
>>>   qemu-options.hx       |  53 ++-
>>>   tpm.c                 |   2 +-
>>>   8 files changed, 1289 insertions(+), 9 deletions(-)
>>>   create mode 100644 hw/tpm/tpm_emulator.c
>>>   create mode 100644 hw/tpm/tpm_ioctl.h
>>> +static int tpm_emulator_spawn_emulator(TPMEmulator *tpm_pt)
>>> +{
>>> +    int fds[2] = { -1, -1 };
>>> +    int ctrl_fds[2] = { -1, -1 };
>>> +    pid_t cpid;
>>> +
>>> +    if (!tpm_pt->ops->has_data_path) {
>>> +        if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) < 0) {
>>> +            return -1;
>>> +        }
>>> +    }
>>> +
>>> +    if (!tpm_pt->ops->has_ctrl_path) {
>>> +        if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, ctrl_fds) < 0) {
>>> +            if (!tpm_pt->ops->has_data_path) {
>>> +                closesocket(fds[0]);
>>> +                closesocket(fds[1]);
>>> +            }
>>> +            return -1;
>>> +        }
>>> +    }
>>> +
>>> +    cpid = qemu_fork(NULL);
>>> +    if (cpid < 0) {
>>> +        error_report("tpm-emulator: Fork failure: %s", 
>>> strerror(errno));
>>> +        if (!tpm_pt->ops->has_data_path) {
>>> +            closesocket(fds[0]);
>>> +            closesocket(fds[1]);
>>> +        }
>>> +        if (!tpm_pt->ops->has_ctrl_path) {
>>> +            closesocket(ctrl_fds[0]);
>>> +            closesocket(ctrl_fds[1]);
>>> +        }
>>> +        return -1;
>>> +    }
>>> +
>>> +    if (cpid == 0) { /* CHILD */
>>> +        enum {
>>> +            PARAM_PATH,
>>> +            PARAM_IFACE,
>>> +            PARAM_SERVER,  PARAM_SERVER_ARGS,
>>> +            PARAM_CTRL,    PARAM_CTRL_ARGS,
>>> +            PARAM_STATE,   PARAM_STATE_ARGS,
>>> +            PARAM_PIDFILE, PARAM_PIDFILE_ARGS,
>>> +            PARAM_LOG,     PARAM_LOG_ARGS,
>>> +            PARAM_MAX
>>> +        };
>>> +
>>> +        int i;
>>> +        int data_fd = -1, ctrl_fd = -1;
>>> +        char *argv[PARAM_MAX+1];
>>> +
>>> +        /* close all unused inherited sockets */
>>> +        if (fds[0] >= 0)
>>> +            closesocket(fds[0]);
>>> +        if (ctrl_fds[0] >= 0)
>>> +            closesocket(ctrl_fds[0]);
>> The 'if' checks are pointless - its already guaranteed by the
>> fact you check socketpair() status.
> socketpairs might not be created in case of data-path & ctrl-path 
> provided, so i feel these checks are needed.
>>
>>> +        i = STDERR_FILENO + 1;
>>> +        if (fds[1] >= 0) {
>>> +            data_fd = dup2(fds[1], i++);
>>> +            if (data_fd < 0) {
>>> +                error_report("tpm-emulator: dup2() failure - %s",
>>> +                             strerror(errno));
>>> +                goto exit_child;
>>> +            }
>>> +        }
>>> +        if (ctrl_fds[1] >= 0) {
>>> +            ctrl_fd = dup2(ctrl_fds[1], i++);
>>> +            if (ctrl_fd < 0) {
>>> +                error_report("tpm-emulator: dup2() failure - %s",
>>> +                             strerror(errno));
>>> +                goto exit_child;
>>> +            }
>>> +        }
>>> +        for ( ; i < _SC_OPEN_MAX; i++) {
>> Errr, _SC_OPEN_MAX is not the maximum number of FDs - it is parameter to
>> use with sysconf() to query the number of files - you must call 
>> sysconf().
> Ya, thanks for educating me, i will change this.
>>
>>> +            closesocket(i);
>> close, not closesocket - you can't assume these are all sockets.
> Does this change makes any difference, as per 
> include/sysemu/os-posix.h,  closesocket() is define as close(), and 
> this backend is targeted only for "Linux" targets. Please let me know 
> if i am missing something.
>>
>>
>>> +        DPRINT("\n")
>>> +        if (execv(tpm_pt->ops->path, (char * const *)argv) < 0) {
>>> +            error_report("execv() failure : %s", strerror(errno));
>>> +        }
>>> +
>>> +exit_child:
>>> +        g_strfreev(argv);
>>> +        if (data_fd >= 0)
>>> +            closesocket(data_fd);
>>> +        if (ctrl_fd >= 0)
>>> +            closesocket(ctrl_fd);
>>> +
>>> +        exit(0);
>> You need _exit(), not exit() as you don't want to run atexit() handlers
>> here. You also want '1' not '0' since this is a failure scenario.
> Sure, i will change this.
>>
>>> +    } else { /* self */
>>> +        struct stat st;
>>> +        DPRINTF("child pid: %d", cpid);
>>> +        int rc;
>>> +        int timeout = 3; /* wait for max 3 seconds */
>>> +
>>> +        /* close unsed sockets */
>>> +        if (fds[1] >= 0)
>>> +            closesocket(fds[1]);
>>> +        if (ctrl_fds[1] >= 0)
>>> +            closesocket(ctrl_fds[1]);
>>> +
>>> +        while((rc = stat(TPM_EMULATOR_PIDFILE, &st)) < 0 && 
>>> timeout--) {
>>> +            sleep(1);
>>> +        }
>> A fixed 3 second timeout will inevitably cause failures on systems with
>> high load.
>>
>> Presumably you're trying to handle the scenario where the child process
>> exits without creating the pid file ?
> The check is mainly to know if the child setup is done and ready to 
> accept requests.
>>
>> In which case you can use 'kill(cpid, 0)' and if errno == ESRCH
>> then the child has exited.
>>
>>> +
>>> +        if (timeout == -1) {
>>> +            error_report("tpm-emulator: failed to find pid file: %s",
>>> +                         strerror(errno));
>>> +            goto err_kill_child;
>>> +        }
>>> +
>>> +        tpm_pt->data_ioc = _iochannel_new(tpm_pt->ops->data_path, 
>>> fds[0], NULL);
>>> +        if (!tpm_pt->data_ioc) {
>>> +            error_report("tpm-emulator: Unable to connect socket : 
>>> %s",
>>> +                          tpm_pt->ops->data_path);
>>> +            goto err_kill_child;
>>> +        }
>>> +
>>> +        tpm_pt->ctrl_ioc = _iochannel_new(tpm_pt->ops->ctrl_path, 
>>> ctrl_fds[0], NULL);
>>> +        if (!tpm_pt->ctrl_ioc) {
>>> +            error_report("tpm-emulator: Unable to connect socket : 
>>> %s",
>>> +                          tpm_pt->ops->ctrl_path);
>>> +            goto err_kill_child;
>>> +        }
>>> +
>>> +        tpm_pt->child_running = true;
>>> +
>>> +        qemu_add_child_watch(cpid);
>>> +
>>> +        qio_channel_add_watch(tpm_pt->data_ioc, G_IO_HUP|G_IO_ERR,
>>> +                              tpm_emulator_fd_handler, tpm_pt, NULL);
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err_kill_child:
>>> +    kill(cpid, SIGTERM);
>>> +    closesocket(fds[0]);
>>> +    closesocket(ctrl_fds[0]);
>> You can't assume the child has gone after a SIGTERM. You need to
>> check, and be prepared to SIGKILL after time reasonable time,
>> if needed.
> Ok, i will add this check.
>>
>>> +    tpm_pt->child_running = false;
>>> +
>>> +    return -1;
>>> +}
>>> +
>>> +static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
>>> +    TPM_STANDARD_CMDLINE_OPTS,
>>> +    {
>>> +        .name = "tpmstatedir",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "TPM state directroy",
>>> +    },
>>> +    {
>>> +        .name = "spawn",
>>> +        .type = QEMU_OPT_BOOL,
>>> +        .help = "Wether to spwan given emlatory binary",
>>> +    },
>>> +    {
>>> +        .name = "path",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "Path to TPM emulator binary",
>>> +    },
>>> +    {
>>> +        .name = "data-path",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "Socket path to use for data exhange",
>>> +    },
>>> +    {
>>> +        .name = "ctrl-path",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "Socket path to use for out-of-band control messages",
>>> +    },
>> I'm still not convinced by the need for 2 separate UNIX sockets, unless
>> there's a performance reason, but that looks unlikely.
> I myself, is not expert of swtpm's interface, hence i cannot defend this.
> i am just trying to enable emulation support in qemu using swtpm. 
> Stefan Berger is the right person to comment on this.
>

I don't understand what the issue is with running the control and data 
channels over different file descriptors.