[Qemu-devel] [PULL v2 0/5] Merge tpm 2018/10/29 v2

[Qemu-devel] [PULL v2 0/5] Merge tpm 2018/10/29 v2

[Stefan Berger]

This pull request fixes a couple of TPM support related issues,
such as full initialization of a variable to quiet down valgrind,
a possible race in the TPM related test cases and marking test
cases as skipped if swtpm was not found in PATH.

   Stefan


The following changes since commit a4d710251fa5aa9ec26de4626f11c78500195d12:

  Merge remote-tracking branch 'remotes/berrange/tags/qcrypto-next-pull-request' into staging (2018-10-24 22:08:42 +0100)

are available in the Git repository at:

  git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-10-29-2

for you to fetch changes up to eff1fe9fd0cebe2293eea9597616f792b6b5ad18:

  tpm: Zero-init structure to avoid uninitialized variables in valgrind log (2018-10-30 17:34:22 -0400)

----------------------------------------------------------------
Marc-André Lureau (2):
      tests/tpm: fix tpm_util_swtpm_has_tpm2()
      tests/tpm: Display if swtpm is not found or --tpm2 not supported

Stefan Berger (3):
      docs: tpm: Mention implemented TPM CRB interface emulation and specs
      MAINTAINERS: Change my email address to the new domain
      tpm: Zero-init structure to avoid uninitialized variables in valgrind log

 MAINTAINERS           |  2 +-
 docs/specs/tpm.txt    | 15 +++++++++++++++
 hw/tpm/tpm_emulator.c |  1 +
 tests/tpm-tests.c     | 33 +++++++++++++++++++++------------
 tests/tpm-util.c      | 52 ++++++++++++++++++++--------------------------------
 tests/tpm-util.h      |  2 ++
 6 files changed, 60 insertions(+), 45 deletions(-)

-- 
2.17.1

[Qemu-devel] [PULL v2 1/5] tests/tpm: fix tpm_util_swtpm_has_tpm2()

[Stefan Berger]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Using g_spawn_async_with_pipes() is more complicated than running the
sync version. The async version returns a file descriptor for stdout, which may
not be fully read. Sometime "--tpm2" will failed to be read, and will
cause the related test to be silently skipped.

Use g_spawn_sync() instead, simplifying the code and fixing the race.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 tests/tpm-util.c | 46 ++++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index 9f3f156e42..ae4aaf35ca 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -145,39 +145,33 @@ void tpm_util_pcrread(QTestState *s, tx_func *tx,
     g_assert_cmpmem(buffer, exp_resp_size, exp_resp, exp_resp_size);
 }
 
-static gboolean tpm_util_swtpm_has_tpm2(void)
+static bool tpm_util_swtpm_has_tpm2(void)
 {
-    gint mystdout;
-    gboolean succ;
-    unsigned i;
-    char buffer[10240];
-    ssize_t n;
-    gchar *swtpm_argv[] = {
-        g_strdup("swtpm"), g_strdup("socket"), g_strdup("--help"), NULL
+    bool has_tpm2 = false;
+    char *out = NULL;
+    static const char *argv[] = {
+        "swtpm", "socket", "--help", NULL
     };
 
-    succ = g_spawn_async_with_pipes(NULL, swtpm_argv, NULL,
-                                    G_SPAWN_SEARCH_PATH, NULL, NULL, NULL,
-                                    NULL, &mystdout, NULL, NULL);
-    if (!succ) {
-        goto cleanup;
+    if (!g_spawn_sync(NULL /* working_dir */,
+                      (char **)argv,
+                      NULL /* envp */,
+                      G_SPAWN_SEARCH_PATH,
+                      NULL /* child_setup */,
+                      NULL /* user_data */,
+                      &out,
+                      NULL /* err */,
+                      NULL /* exit_status */,
+                      NULL)) {
+        return false;
     }
 
-    n = read(mystdout, buffer, sizeof(buffer) - 1);
-    if (n < 0) {
-        goto cleanup;
-    }
-    buffer[n] = 0;
-    if (!strstr(buffer, "--tpm2")) {
-        succ = false;
-    }
-
- cleanup:
-    for (i = 0; swtpm_argv[i]; i++) {
-        g_free(swtpm_argv[i]);
+    if (strstr(out, "--tpm2")) {
+        has_tpm2 = true;
     }
 
-    return succ;
+    g_free(out);
+    return has_tpm2;
 }
 
 gboolean tpm_util_swtpm_start(const char *path, GPid *pid,
-- 
2.17.1

[Qemu-devel] [PULL v2 2/5] tests/tpm: Display if swtpm is not found or --tpm2 not supported

[Stefan Berger]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

If swtpm is not found in $PATH or --tpm2 isn't supported, we display
this in the test log. We cannot mark the test as skipped due to a bug in
certain versions of the gtester environment that interprets a skipped test
as failure.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 tests/tpm-tests.c | 33 +++++++++++++++++++++------------
 tests/tpm-util.c  |  8 +-------
 tests/tpm-util.h  |  2 ++
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/tests/tpm-tests.c b/tests/tpm-tests.c
index 10c6592aac..93a5beba01 100644
--- a/tests/tpm-tests.c
+++ b/tests/tpm-tests.c
@@ -18,6 +18,17 @@
 #include "libqtest.h"
 #include "tpm-tests.h"
 
+static bool
+tpm_test_swtpm_skip(void)
+{
+    if (!tpm_util_swtpm_has_tpm2()) {
+        fprintf(stderr, "swtpm not in PATH or missing --tpm2 support; ");
+        return true;
+    }
+
+    return false;
+}
+
 void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
                          const char *ifmodel)
 {
@@ -28,12 +39,13 @@ void tpm_test_swtpm_test(const char *src_tpm_path, tx_func *tx,
     GPid swtpm_pid;
     GError *error = NULL;
 
-    succ = tpm_util_swtpm_start(src_tpm_path, &swtpm_pid, &addr, &error);
-    /* succ may be false if swtpm is not available */
-    if (!succ) {
+    if (tpm_test_swtpm_skip()) {
         return;
     }
 
+    succ = tpm_util_swtpm_start(src_tpm_path, &swtpm_pid, &addr, &error);
+    g_assert_true(succ);
+
     args = g_strdup_printf(
         "-chardev socket,id=chr,path=%s "
         "-tpmdev emulator,id=dev,chardev=chr "
@@ -74,19 +86,17 @@ void tpm_test_swtpm_migration_test(const char *src_tpm_path,
     GError *error = NULL;
     QTestState *src_qemu, *dst_qemu;
 
-    succ = tpm_util_swtpm_start(src_tpm_path, &src_tpm_pid,
-                                &src_tpm_addr, &error);
-    /* succ may be false if swtpm is not available */
-    if (!succ) {
+    if (tpm_test_swtpm_skip()) {
         return;
     }
 
+    succ = tpm_util_swtpm_start(src_tpm_path, &src_tpm_pid,
+                                &src_tpm_addr, &error);
+    g_assert_true(succ);
+
     succ = tpm_util_swtpm_start(dst_tpm_path, &dst_tpm_pid,
                                 &dst_tpm_addr, &error);
-    /* succ may be false if swtpm is not available */
-    if (!succ) {
-        goto err_src_tpm_kill;
-    }
+    g_assert_true(succ);
 
     tpm_util_migration_start_qemu(&src_qemu, &dst_qemu,
                                   src_tpm_addr, dst_tpm_addr, uri,
@@ -118,7 +128,6 @@ void tpm_test_swtpm_migration_test(const char *src_tpm_path,
         qapi_free_SocketAddress(dst_tpm_addr);
     }
 
-err_src_tpm_kill:
     tpm_util_swtpm_kill(src_tpm_pid);
     if (src_tpm_addr) {
         g_unlink(src_tpm_addr->u.q_unix.path);
diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index ae4aaf35ca..e08b137651 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -145,7 +145,7 @@ void tpm_util_pcrread(QTestState *s, tx_func *tx,
     g_assert_cmpmem(buffer, exp_resp_size, exp_resp, exp_resp_size);
 }
 
-static bool tpm_util_swtpm_has_tpm2(void)
+bool tpm_util_swtpm_has_tpm2(void)
 {
     bool has_tpm2 = false;
     char *out = NULL;
@@ -190,11 +190,6 @@ gboolean tpm_util_swtpm_start(const char *path, GPid *pid,
     gboolean succ;
     unsigned i;
 
-    succ = tpm_util_swtpm_has_tpm2();
-    if (!succ) {
-        goto cleanup;
-    }
-
     *addr = g_new0(SocketAddress, 1);
     (*addr)->type = SOCKET_ADDRESS_TYPE_UNIX;
     (*addr)->u.q_unix.path = g_build_filename(path, "sock", NULL);
@@ -202,7 +197,6 @@ gboolean tpm_util_swtpm_start(const char *path, GPid *pid,
     succ = g_spawn_async(NULL, swtpm_argv, NULL, G_SPAWN_SEARCH_PATH,
                          NULL, NULL, pid, error);
 
-cleanup:
     for (i = 0; swtpm_argv[i]; i++) {
         g_free(swtpm_argv[i]);
     }
diff --git a/tests/tpm-util.h b/tests/tpm-util.h
index 330b9657fe..9e98bc5124 100644
--- a/tests/tpm-util.h
+++ b/tests/tpm-util.h
@@ -32,6 +32,8 @@ void tpm_util_pcrextend(QTestState *s, tx_func *tx);
 void tpm_util_pcrread(QTestState *s, tx_func *tx,
                       const unsigned char *exp_resp, size_t exp_resp_size);
 
+bool tpm_util_swtpm_has_tpm2(void);
+
 gboolean tpm_util_swtpm_start(const char *path, GPid *pid,
                               SocketAddress **addr, GError **error);
 void tpm_util_swtpm_kill(GPid pid);
-- 
2.17.1

[Qemu-devel] [PULL v2 3/5] docs: tpm: Mention implemented TPM CRB interface emulation and specs

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Add a few sentences about the implemented emulation of the TPM CRB
interface and its specification.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 docs/specs/tpm.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index 0e9bbebe1d..1af82bba86 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -20,6 +20,21 @@ QEMU files related to TPM TIS interface:
  - hw/tpm/tpm_tis.h
 
 
+QEMU also implements a TPM CRB interface following the Trusted Computing
+Group's specification "TCG PC Client Platform TPM Profile (PTP)
+Specification", Family "2.0", Level 00 Revision 01.03 v22, May 22, 2017.
+This specification, or a later version of it, can be accessed from the
+following URL:
+
+https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/
+
+The CRB interface makes a memory mapped IO region in the area 0xfed40000 -
+0xfed40fff (1 locality) available to the guest operating system.
+
+QEMU files related to TPM CRB interface:
+ - hw/tpm/tpm_crb.c
+
+
 = ACPI Interface =
 
 The TPM device is defined with ACPI ID "PNP0C31". QEMU builds a SSDT and passes
-- 
2.17.1

[Qemu-devel] [PULL v2 4/5] MAINTAINERS: Change my email address to the new domain

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

My old email address will soon not work anymore, so change it to the
new domain.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 40672c4eba..332353ce5c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1794,7 +1794,7 @@ F: docs/devel/tracing.txt
 T: git git://github.com/stefanha/qemu.git tracing
 
 TPM
-M: Stefan Berger <stefanb@linux.vnet.ibm.com>
+M: Stefan Berger <stefanb@linux.ibm.com>
 S: Maintained
 F: tpm.c
 F: stubs/tpm.c
-- 
2.17.1

[Qemu-devel] [PULL v2 5/5] tpm: Zero-init structure to avoid uninitialized variables in valgrind log

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Zero-init the ptm_loc structure so that we don't have fields that
are not initialised.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/tpm/tpm_emulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 10bc20dbec..70f4b10284 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -166,6 +166,7 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, uint8_t locty_number,
 
     trace_tpm_emulator_set_locality(locty_number);
 
+    memset(&loc, 0, sizeof(loc));
     loc.u.req.loc = locty_number;
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_LOCALITY, &loc,
                              sizeof(loc), sizeof(loc)) < 0) {
-- 
2.17.1

Re: [Qemu-devel] [PULL v2 0/5] Merge tpm 2018/10/29 v2

[Peter Maydell]

On 30 October 2018 at 21:47, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> This pull request fixes a couple of TPM support related issues,
> such as full initialization of a variable to quiet down valgrind,
> a possible race in the TPM related test cases and marking test
> cases as skipped if swtpm was not found in PATH.
>
>    Stefan
>
>
> The following changes since commit a4d710251fa5aa9ec26de4626f11c78500195d12:
>
>   Merge remote-tracking branch 'remotes/berrange/tags/qcrypto-next-pull-request' into staging (2018-10-24 22:08:42 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-10-29-2
>
> for you to fetch changes up to eff1fe9fd0cebe2293eea9597616f792b6b5ad18:
>
>   tpm: Zero-init structure to avoid uninitialized variables in valgrind log (2018-10-30 17:34:22 -0400)
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL v2 2/5] tests/tpm: Display if swtpm is not found or --tpm2 not supported

[Thomas Huth]

On 2018-10-30 22:47, Stefan Berger wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> If swtpm is not found in $PATH or --tpm2 isn't supported, we display
> this in the test log. We cannot mark the test as skipped due to a bug in
> certain versions of the gtester environment that interprets a skipped test
> as failure.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  tests/tpm-tests.c | 33 +++++++++++++++++++++------------
>  tests/tpm-util.c  |  8 +-------
>  tests/tpm-util.h  |  2 ++
>  3 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/tpm-tests.c b/tests/tpm-tests.c
> index 10c6592aac..93a5beba01 100644
> --- a/tests/tpm-tests.c
> +++ b/tests/tpm-tests.c
> @@ -18,6 +18,17 @@
>  #include "libqtest.h"
>  #include "tpm-tests.h"
>  
> +static bool
> +tpm_test_swtpm_skip(void)
> +{
> +    if (!tpm_util_swtpm_has_tpm2()) {
> +        fprintf(stderr, "swtpm not in PATH or missing --tpm2 support; ");
> +        return true;
> +    }

I now get this ugly (4-times duplicated) output each time I run "make check":

swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support;

Could you please use g_test_message() here instead like we're doing
it in most of the other tests already?

 Thanks,
  Thomas

Re: [Qemu-devel] [PULL v2 2/5] tests/tpm: Display if swtpm is not found or --tpm2 not supported

[Stefan Berger]

On 11/6/18 6:51 AM, Thomas Huth wrote:
> On 2018-10-30 22:47, Stefan Berger wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> If swtpm is not found in $PATH or --tpm2 isn't supported, we display
>> this in the test log. We cannot mark the test as skipped due to a bug in
>> certain versions of the gtester environment that interprets a skipped test
>> as failure.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   tests/tpm-tests.c | 33 +++++++++++++++++++++------------
>>   tests/tpm-util.c  |  8 +-------
>>   tests/tpm-util.h  |  2 ++
>>   3 files changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/tests/tpm-tests.c b/tests/tpm-tests.c
>> index 10c6592aac..93a5beba01 100644
>> --- a/tests/tpm-tests.c
>> +++ b/tests/tpm-tests.c
>> @@ -18,6 +18,17 @@
>>   #include "libqtest.h"
>>   #include "tpm-tests.h"
>>   
>> +static bool
>> +tpm_test_swtpm_skip(void)
>> +{
>> +    if (!tpm_util_swtpm_has_tpm2()) {
>> +        fprintf(stderr, "swtpm not in PATH or missing --tpm2 support; ");
>> +        return true;
>> +    }
> I now get this ugly (4-times duplicated) output each time I run "make check":
>
> swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support; swtpm not in PATH or missing --tpm2 support;
>
> Could you please use g_test_message() here instead like we're doing
> it in most of the other tests already?


Sure, I'll change it. When I tested it I found these lines in the log 
formatted as expected.

    Stefan

>
>   Thanks,
>    Thomas
>