xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] vtpmmgr: Some fixes - still incomplete
@ 2021-05-04 12:48 Jason Andryuk
  2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
                   ` (8 more replies)
  0 siblings, 9 replies; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Ian Jackson, Wei Liu, Daniel De Graaf, Quan Xu,
	Samuel Thibault, Dag Nygren

vtpmmgr TPM 2.0 support is incomplete.  There is no code to save the
tpm2 keys generated by the vtpmmgr, so it's impossible to restore vtpm
state with tpm2.  The vtpmmgr also issues TPM 1.2 commands to the TPM
2.0 hardware which naturally fails.  Dag reported this [1][2], and I
independently re-discovered it.

I have not fixed the above issues.  These are some fixes I made while
investigating tpm2 support.  At a minimum, "docs: Warn about incomplete
vtpmmgr TPM 2.0 support" should be applied to warn others.

This is useful for debugging:
vtpmmgr: Print error code to aid debugging

This fixes vtpmmgr output (also noted by Dag [3]) but maybe removing %z
would be better:
stubom: newlib: Enable C99 formats for %z

This gives more flexibility if you are already using the TPM2 hardware:
vtpmmgr: Allow specifying srk_handle for TPM2

These are some changes to unload keys from the TPM hardware (so they
are not still loaded for anything that runs afterwards):
vtpmmgr: Move vtpmmgr_shutdown
vtpmmgr: Flush transient keys on shutdown
vtpmmgr: Flush all transient keys
vtpmmgr: Shutdown more gracefully

This lets vtpms initialize their random pools:
vtpmmgr: Support GetRandom passthrough on TPM 2.0

[1] https://lore.kernel.org/xen-devel/8285393.eUs1EhXEQl@eseries.newtech.fi/
[2] https://lore.kernel.org/xen-devel/1615731.eyaQ0j4tC5@eseries.newtech.fi/
[3] https://lore.kernel.org/xen-devel/3151252.0ZAaMuH7Fy@dag.newtech.fi/

Jason Andryuk (9):
  docs: Warn about incomplete vtpmmgr TPM 2.0 support
  vtpmmgr: Print error code to aid debugging
  stubom: newlib: Enable C99 formats for %z
  vtpmmgr: Allow specifying srk_handle for TPM2
  vtpmmgr: Move vtpmmgr_shutdown
  vtpmmgr: Flush transient keys on shutdown
  vtpmmgr: Flush all transient keys
  vtpmmgr: Shutdown more gracefully
  vtpmmgr: Support GetRandom passthrough on TPM 2.0

 docs/man/xen-vtpmmgr.7.pod         | 18 +++++++++++
 stubdom/Makefile                   |  2 +-
 stubdom/vtpmmgr/init.c             | 49 ++++++++++++++++++++----------
 stubdom/vtpmmgr/marshal.h          | 10 ++++++
 stubdom/vtpmmgr/tpm.c              |  2 +-
 stubdom/vtpmmgr/tpm2.c             |  2 +-
 stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 +++++++++++++++++++++++++++++
 stubdom/vtpmmgr/vtpmmgr.c          | 12 +++++++-
 8 files changed, 123 insertions(+), 20 deletions(-)

-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 17:55   ` Andrew Cooper
  2021-05-07 15:31   ` Daniel P. Smith
  2021-05-04 12:48 ` [PATCH 2/9] vtpmmgr: Print error code to aid debugging Jason Andryuk
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Ian Jackson, Wei Liu

The vtpmmgr TPM 2.0 support is incomplete.  Add a warning about that to
the documentation so others don't have to work through discovering it is
broken.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 docs/man/xen-vtpmmgr.7.pod | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod
index af825a7ffe..875dcce508 100644
--- a/docs/man/xen-vtpmmgr.7.pod
+++ b/docs/man/xen-vtpmmgr.7.pod
@@ -222,6 +222,17 @@ XSM label, not the kernel.
 
 =head1 Appendix B: vtpmmgr on TPM 2.0
 
+=head2 WARNING: Incomplete - cannot persist data
+
+TPM 2.0 support for vTPM manager is incomplete.  There is no support for
+persisting an encryption key, so vTPM manager regenerates primary and secondary
+key handles each boot.
+
+Also, the vTPM manger group command implementation hardcodes TPM 1.2 commands.
+This means running manage-vtpmmgr.pl fails when the TPM 2.0 hardware rejects
+the TPM 1.2 commands.  vTPM manager with TPM 2.0 cannot create groups and
+therefore cannot persist vTPM contents.
+
 =head2 Manager disk image setup:
 
 The vTPM Manager requires a disk image to store its encrypted data. The image
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 2/9] vtpmmgr: Print error code to aid debugging
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
  2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:03   ` Samuel Thibault
  2021-05-07 15:33   ` Daniel P. Smith
  2021-05-04 12:48 ` [PATCH 3/9] stubom: newlib: Enable C99 formats for %z Jason Andryuk
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

tpm_get_error_name returns "Unknown Error Code" when an error string
is not defined.  In that case, we should print the Error Code so it can
be looked up offline.  tpm_get_error_name returns a const string, so
just have the two callers always print the error code so it is always
available.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/tpm.c  | 2 +-
 stubdom/vtpmmgr/tpm2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/stubdom/vtpmmgr/tpm.c b/stubdom/vtpmmgr/tpm.c
index 779cddd64e..83b2bc16b2 100644
--- a/stubdom/vtpmmgr/tpm.c
+++ b/stubdom/vtpmmgr/tpm.c
@@ -109,7 +109,7 @@
 			UINT32 rsp_status; \
 			UNPACK_OUT(TPM_RSP_HEADER, &rsp_tag, &rsp_len, &rsp_status); \
 			if (rsp_status != TPM_SUCCESS) { \
-				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(rsp_status)); \
+				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(rsp_status), rsp_status); \
 				status = rsp_status; \
 				goto abort_egress; \
 			} \
diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c
index c9f1016ab5..655e6d164c 100644
--- a/stubdom/vtpmmgr/tpm2.c
+++ b/stubdom/vtpmmgr/tpm2.c
@@ -126,7 +126,7 @@
     ptr = unpack_TPM_RSP_HEADER(ptr, \
           &(tag), &(paramSize), &(status));\
     if ((status) != TPM_SUCCESS){ \
-        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(status));\
+        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(status), (status));\
         goto abort_egress;\
     }\
 } while(0)
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 3/9] stubom: newlib: Enable C99 formats for %z
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
  2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
  2021-05-04 12:48 ` [PATCH 2/9] vtpmmgr: Print error code to aid debugging Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:08   ` Samuel Thibault
  2021-05-07 15:37   ` Daniel P. Smith
  2021-05-04 12:48 ` [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2 Jason Andryuk
                   ` (5 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Ian Jackson, Wei Liu, Samuel Thibault

vtpmmgr was changed to print size_t with the %z modifier, but newlib
isn't compiled with %z support.  So you get output like:

root seal: zu; sector of 13: zu
root: zu v=zu
itree: 36; sector of 112: zu
group: zu v=zu id=zu md=zu
group seal: zu; 5 in parent: zu; sector of 13: zu
vtpm: zu+zu; sector of 48: zu

Enable the C99 formats in newlib so vtpmmgr prints the numeric values.

Fixes 9379af08ccc0 "stubdom: vtpmmgr: Correctly format size_t with %z
when printing."

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
I haven't tried, but the other option would be to cast size_t and avoid
%z.  Since this seems to be the only mini-os use of %z, that may be
better than building a larger newlib.
---
 stubdom/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stubdom/Makefile b/stubdom/Makefile
index 90d9ffcd9f..c6de5f68ae 100644
--- a/stubdom/Makefile
+++ b/stubdom/Makefile
@@ -105,7 +105,7 @@ cross-newlib: $(NEWLIB_STAMPFILE)
 $(NEWLIB_STAMPFILE): mk-headers-$(XEN_TARGET_ARCH) newlib-$(NEWLIB_VERSION)
 	mkdir -p newlib-$(XEN_TARGET_ARCH)
 	( cd newlib-$(XEN_TARGET_ARCH) && \
-	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --disable-multilib && \
+	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --enable-newlib-io-c99-formats --disable-multilib && \
 	  $(MAKE) DESTDIR= && \
 	  $(MAKE) DESTDIR= install )
 
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (2 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 3/9] stubom: newlib: Enable C99 formats for %z Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:13   ` Samuel Thibault
  2021-05-04 12:48 ` [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown Jason Andryuk
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Ian Jackson, Wei Liu, Daniel De Graaf, Quan Xu,
	Samuel Thibault

Bypass taking ownership of the TPM2 if an srk_handle is specified.

This srk_handle must be usable with Null auth for the time being.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 docs/man/xen-vtpmmgr.7.pod |  7 +++++++
 stubdom/vtpmmgr/init.c     | 11 ++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod
index 875dcce508..3286954568 100644
--- a/docs/man/xen-vtpmmgr.7.pod
+++ b/docs/man/xen-vtpmmgr.7.pod
@@ -92,6 +92,13 @@ Valid arguments:
 
 =over 4
 
+=item srk_handle=<HANDLE>
+
+Specify a srk_handle for TPM 2.0.  TPM 2.0 uses a key hierarchy, and
+this allow specifying the parent handle for vtpmmgr to create its own
+key under.  Using this option bypasses vtpmmgr trying to take ownership
+of the TPM.
+
 =item owner_auth=<AUTHSPEC>
 
 =item srk_auth=<AUTHSPEC>
diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
index 1506735051..c01d03e9f4 100644
--- a/stubdom/vtpmmgr/init.c
+++ b/stubdom/vtpmmgr/init.c
@@ -302,6 +302,11 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts)
             goto err_invalid;
          }
       }
+      else if(!strncmp(argv[i], "srk_handle:", 11)) {
+         if(sscanf(argv[i] + 11, "%x", &vtpm_globals.srk_handle) != 1) {
+            goto err_invalid;
+         }
+      }
       else if(!strncmp(argv[i], "tpmdriver=", 10)) {
          if(!strcmp(argv[i] + 10, "tpm_tis")) {
             opts->tpmdriver = TPMDRV_TPM_TIS;
@@ -586,7 +591,11 @@ TPM_RESULT vtpmmgr2_create(void)
 {
     TPM_RESULT status = TPM_SUCCESS;
 
-    TPMTRYRETURN(tpm2_take_ownership());
+    if ( vtpm_globals.srk_handle == 0 ) {
+        TPMTRYRETURN(tpm2_take_ownership());
+    } else {
+        tpm2_AuthArea_ctor(NULL, 0, &vtpm_globals.srk_auth_area);
+    }
 
    /* create SK */
     TPM2_Create_Params_out out;
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (3 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2 Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:14   ` Samuel Thibault
  2021-05-04 12:48 ` [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown Jason Andryuk
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

Reposition vtpmmgr_shutdown so it can call flush_tpm2 without a forward
declaration.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/init.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
index c01d03e9f4..569b0dd1dc 100644
--- a/stubdom/vtpmmgr/init.c
+++ b/stubdom/vtpmmgr/init.c
@@ -503,20 +503,6 @@ egress:
    return status;
 }
 
-void vtpmmgr_shutdown(void)
-{
-   /* Cleanup TPM resources */
-   TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle);
-
-   /* Close tpmback */
-   shutdown_tpmback();
-
-   /* Close tpmfront/tpm_tis */
-   close(vtpm_globals.tpm_fd);
-
-   vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n");
-}
-
 /* TPM 2.0 */
 
 static void tpm2_AuthArea_ctor(const char *authValue, UINT32 authLen,
@@ -797,3 +783,17 @@ abort_egress:
 egress:
     return status;
 }
+
+void vtpmmgr_shutdown(void)
+{
+   /* Cleanup TPM resources */
+   TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle);
+
+   /* Close tpmback */
+   shutdown_tpmback();
+
+   /* Close tpmfront/tpm_tis */
+   close(vtpm_globals.tpm_fd);
+
+   vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n");
+}
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (4 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:15   ` Samuel Thibault
  2021-05-04 12:48 ` [PATCH 7/9] vtpmmgr: Flush all transient keys Jason Andryuk
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

Remove our key so it isn't left in the TPM for someone to come along
after vtpmmgr shutsdown.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/init.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
index 569b0dd1dc..d9fefa9be6 100644
--- a/stubdom/vtpmmgr/init.c
+++ b/stubdom/vtpmmgr/init.c
@@ -792,6 +792,14 @@ void vtpmmgr_shutdown(void)
    /* Close tpmback */
    shutdown_tpmback();
 
+    if (hw_is_tpm2()) {
+        /* Blow away all stale handles left in the tpm*/
+        if (flush_tpm2() != TPM_SUCCESS) {
+            vtpmlogerror(VTPM_LOG_TPM,
+                         "TPM2_FlushResources failed, continuing shutdown..\n");
+        }
+    }
+
    /* Close tpmfront/tpm_tis */
    close(vtpm_globals.tpm_fd);
 
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 7/9] vtpmmgr: Flush all transient keys
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (5 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:16   ` Samuel Thibault
  2021-05-04 12:48 ` [PATCH 8/9] vtpmmgr: Shutdown more gracefully Jason Andryuk
  2021-05-04 12:48 ` [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0 Jason Andryuk
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

We're only flushing 2 transients, but there are 3 handles.  Use <= to also
flush the third handle.

The number of transient handles/keys is hardware dependent, so this
should query for the limit.  And assignment of handles is assumed to be
sequential from the minimum.  That may not be guaranteed, but seems okay
with my tpm2.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
index d9fefa9be6..e0dbcac3ad 100644
--- a/stubdom/vtpmmgr/init.c
+++ b/stubdom/vtpmmgr/init.c
@@ -656,7 +656,7 @@ static TPM_RC flush_tpm2(void)
 {
     int i;
 
-    for (i = TRANSIENT_FIRST; i < TRANSIENT_LAST; i++)
+    for (i = TRANSIENT_FIRST; i <= TRANSIENT_LAST; i++)
          TPM2_FlushContext(i);
 
     return TPM_SUCCESS;
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 8/9] vtpmmgr: Shutdown more gracefully
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (6 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 7/9] vtpmmgr: Flush all transient keys Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:18   ` Samuel Thibault
  2021-05-04 12:48 ` [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0 Jason Andryuk
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

vtpmmgr uses the default, weak app_shutdown, which immediately calls the
shutdown hypercall.  This short circuits the vtpmmgr clean up logic.  We
need to perform the clean up to actually Flush our key out of the tpm.

Setting do_shutdown is one step in that direction, but vtpmmgr will most
likely be waiting in tpmback_req_any.  We need to call shutdown_tpmback
to cancel the wait inside tpmback and perform the shutdown.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/vtpmmgr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/stubdom/vtpmmgr/vtpmmgr.c b/stubdom/vtpmmgr/vtpmmgr.c
index 9fddaa24f8..46ea018921 100644
--- a/stubdom/vtpmmgr/vtpmmgr.c
+++ b/stubdom/vtpmmgr/vtpmmgr.c
@@ -67,11 +67,21 @@ int hw_is_tpm2(void)
     return (hardware_version.hw_version == TPM2_HARDWARE) ? 1 : 0;
 }
 
+static int do_shutdown;
+
+void app_shutdown(unsigned int reason)
+{
+    printk("Shutdown requested: %d\n", reason);
+    do_shutdown = 1;
+
+    shutdown_tpmback();
+}
+
 void main_loop(void) {
    tpmcmd_t* tpmcmd;
    uint8_t respbuf[TCPA_MAX_BUFFER_LENGTH];
 
-   while(1) {
+   while (!do_shutdown) {
       /* Wait for requests from a vtpm */
       vtpmloginfo(VTPM_LOG_VTPM, "Waiting for commands from vTPM's:\n");
       if((tpmcmd = tpmback_req_any()) == NULL) {
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0
  2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
                   ` (7 preceding siblings ...)
  2021-05-04 12:48 ` [PATCH 8/9] vtpmmgr: Shutdown more gracefully Jason Andryuk
@ 2021-05-04 12:48 ` Jason Andryuk
  2021-05-04 13:33   ` Samuel Thibault
  8 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 12:48 UTC (permalink / raw)
  To: xen-devel; +Cc: Jason Andryuk, Daniel De Graaf, Quan Xu, Samuel Thibault

GetRandom passthrough currently fails when using vtpmmgr with a hardware
TPM 2.0.
vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom
vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30)

When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2
TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command.  Besides the
differing ordinal, the TPM 1.2 uses 32bit sizes for the request and
response (vs. 16bit for TPM2).

Place the random output directly into the tpmcmd->resp and build the
packet around it.  This avoids bouncing through an extra buffer, but the
header has to be written after grabbing the random bytes so we have the
number of bytes to include in the size.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 stubdom/vtpmmgr/marshal.h          | 10 +++++++
 stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h
index dce19c6439..20da22af09 100644
--- a/stubdom/vtpmmgr/marshal.h
+++ b/stubdom/vtpmmgr/marshal.h
@@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) {
 	return rv;
 }
 
+static
+inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) {
+	int rv = 0;
+	rv += sizeof_UINT16(ptr);
+	rv += sizeof_UINT32(ptr);
+	rv += sizeof_UINT32(ptr);
+	return rv;
+}
+
 static
 inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr,
 		TPM_TAG tag,
@@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max,
 #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r)
 #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r)
 #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r)
+#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p)
 
 #endif
diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c
index 2ac14fae77..7ca1d9df94 100644
--- a/stubdom/vtpmmgr/vtpm_cmd_handler.c
+++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c
@@ -47,6 +47,7 @@
 #include "vtpm_disk.h"
 #include "vtpmmgr.h"
 #include "tpm.h"
+#include "tpm2.h"
 #include "tpmrsa.h"
 #include "tcg.h"
 #include "mgmt_authority.h"
@@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq)
 	return 1;
 }
 
+TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque,
+				    tpmcmd_t* tpmcmd)
+{
+	TPM_RESULT status = TPM_SUCCESS;
+	TPM_TAG tag;
+	UINT32 size;
+	UINT32 rand_offset;
+	UINT32 rand_size;
+	TPM_COMMAND_CODE ord;
+	BYTE *p;
+
+	p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord);
+
+	if (!hw_is_tpm2()) {
+		size = TCPA_MAX_BUFFER_LENGTH;
+		TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len,
+					      tpmcmd->resp, &size));
+		tpmcmd->resp_len = size;
+
+		return TPM_SUCCESS;
+	}
+
+	/* TPM_GetRandom req: <header><uint32 num bytes> */
+	unpack_UINT32(p, &rand_size);
+
+	/* Call TPM2_GetRandom but return a TPM_GetRandom response. */
+	/* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */
+        rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) +
+		      sizeof_UINT32(tpmcmd->resp);
+
+	TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset));
+
+	p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND,
+				rand_offset + rand_size, status);
+	p = pack_UINT32(p, rand_size);
+	tpmcmd->resp_len = rand_offset + rand_size;
+
+	return status;
+
+abort_egress:
+	tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE;
+	pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status);
+
+	return status;
+}
+
 TPM_RESULT vtpmmgr_handle_cmd(
 		struct tpm_opaque *opaque,
 		tpmcmd_t* tpmcmd)
@@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd(
 		switch(ord) {
 		case TPM_ORD_GetRandom:
 			vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n");
+			return vtpmmgr_handle_getrandom(opaque, tpmcmd);
 			break;
 		case TPM_ORD_PcrRead:
 			vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n");
-- 
2.30.2



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/9] vtpmmgr: Print error code to aid debugging
  2021-05-04 12:48 ` [PATCH 2/9] vtpmmgr: Print error code to aid debugging Jason Andryuk
@ 2021-05-04 13:03   ` Samuel Thibault
  2021-05-07 15:33   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:03 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:35 -0400, a ecrit:
> tpm_get_error_name returns "Unknown Error Code" when an error string
> is not defined.  In that case, we should print the Error Code so it can
> be looked up offline.  tpm_get_error_name returns a const string, so
> just have the two callers always print the error code so it is always
> available.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  stubdom/vtpmmgr/tpm.c  | 2 +-
>  stubdom/vtpmmgr/tpm2.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/stubdom/vtpmmgr/tpm.c b/stubdom/vtpmmgr/tpm.c
> index 779cddd64e..83b2bc16b2 100644
> --- a/stubdom/vtpmmgr/tpm.c
> +++ b/stubdom/vtpmmgr/tpm.c
> @@ -109,7 +109,7 @@
>  			UINT32 rsp_status; \
>  			UNPACK_OUT(TPM_RSP_HEADER, &rsp_tag, &rsp_len, &rsp_status); \
>  			if (rsp_status != TPM_SUCCESS) { \
> -				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(rsp_status)); \
> +				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(rsp_status), rsp_status); \
>  				status = rsp_status; \
>  				goto abort_egress; \
>  			} \
> diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c
> index c9f1016ab5..655e6d164c 100644
> --- a/stubdom/vtpmmgr/tpm2.c
> +++ b/stubdom/vtpmmgr/tpm2.c
> @@ -126,7 +126,7 @@
>      ptr = unpack_TPM_RSP_HEADER(ptr, \
>            &(tag), &(paramSize), &(status));\
>      if ((status) != TPM_SUCCESS){ \
> -        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(status));\
> +        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(status), (status));\
>          goto abort_egress;\
>      }\
>  } while(0)
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/9] stubom: newlib: Enable C99 formats for %z
  2021-05-04 12:48 ` [PATCH 3/9] stubom: newlib: Enable C99 formats for %z Jason Andryuk
@ 2021-05-04 13:08   ` Samuel Thibault
  2021-05-07 15:37   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:08 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu

Jason Andryuk, le mar. 04 mai 2021 08:48:36 -0400, a ecrit:
> vtpmmgr was changed to print size_t with the %z modifier, but newlib
> isn't compiled with %z support.  So you get output like:
> 
> root seal: zu; sector of 13: zu
> root: zu v=zu
> itree: 36; sector of 112: zu
> group: zu v=zu id=zu md=zu
> group seal: zu; 5 in parent: zu; sector of 13: zu
> vtpm: zu+zu; sector of 48: zu
> 
> Enable the C99 formats in newlib so vtpmmgr prints the numeric values.
> 
> Fixes 9379af08ccc0 "stubdom: vtpmmgr: Correctly format size_t with %z
> when printing."
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
> I haven't tried, but the other option would be to cast size_t and avoid
> %z.  Since this seems to be the only mini-os use of %z, that may be
> better than building a larger newlib.

The size difference will be very small. I believe we prefer to have a
working %z rather than getting surprised to see it non-working.

> ---
>  stubdom/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/stubdom/Makefile b/stubdom/Makefile
> index 90d9ffcd9f..c6de5f68ae 100644
> --- a/stubdom/Makefile
> +++ b/stubdom/Makefile
> @@ -105,7 +105,7 @@ cross-newlib: $(NEWLIB_STAMPFILE)
>  $(NEWLIB_STAMPFILE): mk-headers-$(XEN_TARGET_ARCH) newlib-$(NEWLIB_VERSION)
>  	mkdir -p newlib-$(XEN_TARGET_ARCH)
>  	( cd newlib-$(XEN_TARGET_ARCH) && \
> -	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --disable-multilib && \
> +	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --enable-newlib-io-c99-formats --disable-multilib && \
>  	  $(MAKE) DESTDIR= && \
>  	  $(MAKE) DESTDIR= install )
>  
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 12:48 ` [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2 Jason Andryuk
@ 2021-05-04 13:13   ` Samuel Thibault
  2021-05-04 17:04     ` Jason Andryuk
  0 siblings, 1 reply; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:13 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:37 -0400, a ecrit:
> Bypass taking ownership of the TPM2 if an srk_handle is specified.
> 
> This srk_handle must be usable with Null auth for the time being.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  docs/man/xen-vtpmmgr.7.pod |  7 +++++++
>  stubdom/vtpmmgr/init.c     | 11 ++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod
> index 875dcce508..3286954568 100644
> --- a/docs/man/xen-vtpmmgr.7.pod
> +++ b/docs/man/xen-vtpmmgr.7.pod
> @@ -92,6 +92,13 @@ Valid arguments:
>  
>  =over 4
>  
> +=item srk_handle=<HANDLE>

Is this actually srk_handle= or srk_handle: ?

The code tests for the latter. The problem seems to "exist" also for
owner_auth: and srk_auth: but both = and : work actually because strncmp
is told not to check for = and :

We'd better clean this up to avoid confusions.

Samuel

> +
> +Specify a srk_handle for TPM 2.0.  TPM 2.0 uses a key hierarchy, and
> +this allow specifying the parent handle for vtpmmgr to create its own
> +key under.  Using this option bypasses vtpmmgr trying to take ownership
> +of the TPM.
> +
>  =item owner_auth=<AUTHSPEC>
>  
>  =item srk_auth=<AUTHSPEC>
> diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
> index 1506735051..c01d03e9f4 100644
> --- a/stubdom/vtpmmgr/init.c
> +++ b/stubdom/vtpmmgr/init.c
> @@ -302,6 +302,11 @@ int parse_cmdline_opts(int argc, char** argv, struct Opts* opts)
>              goto err_invalid;
>           }
>        }
> +      else if(!strncmp(argv[i], "srk_handle:", 11)) {
> +         if(sscanf(argv[i] + 11, "%x", &vtpm_globals.srk_handle) != 1) {
> +            goto err_invalid;
> +         }
> +      }
>        else if(!strncmp(argv[i], "tpmdriver=", 10)) {
>           if(!strcmp(argv[i] + 10, "tpm_tis")) {
>              opts->tpmdriver = TPMDRV_TPM_TIS;
> @@ -586,7 +591,11 @@ TPM_RESULT vtpmmgr2_create(void)
>  {
>      TPM_RESULT status = TPM_SUCCESS;
>  
> -    TPMTRYRETURN(tpm2_take_ownership());
> +    if ( vtpm_globals.srk_handle == 0 ) {
> +        TPMTRYRETURN(tpm2_take_ownership());
> +    } else {
> +        tpm2_AuthArea_ctor(NULL, 0, &vtpm_globals.srk_auth_area);
> +    }
>  
>     /* create SK */
>      TPM2_Create_Params_out out;
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown
  2021-05-04 12:48 ` [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown Jason Andryuk
@ 2021-05-04 13:14   ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:14 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:38 -0400, a ecrit:
> Reposition vtpmmgr_shutdown so it can call flush_tpm2 without a forward
> declaration.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  stubdom/vtpmmgr/init.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
> index c01d03e9f4..569b0dd1dc 100644
> --- a/stubdom/vtpmmgr/init.c
> +++ b/stubdom/vtpmmgr/init.c
> @@ -503,20 +503,6 @@ egress:
>     return status;
>  }
>  
> -void vtpmmgr_shutdown(void)
> -{
> -   /* Cleanup TPM resources */
> -   TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle);
> -
> -   /* Close tpmback */
> -   shutdown_tpmback();
> -
> -   /* Close tpmfront/tpm_tis */
> -   close(vtpm_globals.tpm_fd);
> -
> -   vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n");
> -}
> -
>  /* TPM 2.0 */
>  
>  static void tpm2_AuthArea_ctor(const char *authValue, UINT32 authLen,
> @@ -797,3 +783,17 @@ abort_egress:
>  egress:
>      return status;
>  }
> +
> +void vtpmmgr_shutdown(void)
> +{
> +   /* Cleanup TPM resources */
> +   TPM_TerminateHandle(vtpm_globals.oiap.AuthHandle);
> +
> +   /* Close tpmback */
> +   shutdown_tpmback();
> +
> +   /* Close tpmfront/tpm_tis */
> +   close(vtpm_globals.tpm_fd);
> +
> +   vtpmloginfo(VTPM_LOG_VTPM, "VTPM Manager stopped.\n");
> +}
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown
  2021-05-04 12:48 ` [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown Jason Andryuk
@ 2021-05-04 13:15   ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:15 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:39 -0400, a ecrit:
> Remove our key so it isn't left in the TPM for someone to come along
> after vtpmmgr shutsdown.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  stubdom/vtpmmgr/init.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
> index 569b0dd1dc..d9fefa9be6 100644
> --- a/stubdom/vtpmmgr/init.c
> +++ b/stubdom/vtpmmgr/init.c
> @@ -792,6 +792,14 @@ void vtpmmgr_shutdown(void)
>     /* Close tpmback */
>     shutdown_tpmback();
>  
> +    if (hw_is_tpm2()) {
> +        /* Blow away all stale handles left in the tpm*/
> +        if (flush_tpm2() != TPM_SUCCESS) {
> +            vtpmlogerror(VTPM_LOG_TPM,
> +                         "TPM2_FlushResources failed, continuing shutdown..\n");
> +        }
> +    }
> +
>     /* Close tpmfront/tpm_tis */
>     close(vtpm_globals.tpm_fd);
>  
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/9] vtpmmgr: Flush all transient keys
  2021-05-04 12:48 ` [PATCH 7/9] vtpmmgr: Flush all transient keys Jason Andryuk
@ 2021-05-04 13:16   ` Samuel Thibault
  2021-05-04 17:05     ` Jason Andryuk
  0 siblings, 1 reply; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:16 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:40 -0400, a ecrit:
> We're only flushing 2 transients, but there are 3 handles.  Use <= to also
> flush the third handle.
> 
> The number of transient handles/keys is hardware dependent, so this
> should query for the limit.  And assignment of handles is assumed to be
> sequential from the minimum.  That may not be guaranteed, but seems okay
> with my tpm2.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Maybe explicit in the log that TRANSIENT_LAST is actually inclusive?

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  stubdom/vtpmmgr/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/stubdom/vtpmmgr/init.c b/stubdom/vtpmmgr/init.c
> index d9fefa9be6..e0dbcac3ad 100644
> --- a/stubdom/vtpmmgr/init.c
> +++ b/stubdom/vtpmmgr/init.c
> @@ -656,7 +656,7 @@ static TPM_RC flush_tpm2(void)
>  {
>      int i;
>  
> -    for (i = TRANSIENT_FIRST; i < TRANSIENT_LAST; i++)
> +    for (i = TRANSIENT_FIRST; i <= TRANSIENT_LAST; i++)
>           TPM2_FlushContext(i);
>  
>      return TPM_SUCCESS;
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/9] vtpmmgr: Shutdown more gracefully
  2021-05-04 12:48 ` [PATCH 8/9] vtpmmgr: Shutdown more gracefully Jason Andryuk
@ 2021-05-04 13:18   ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:18 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:41 -0400, a ecrit:
> vtpmmgr uses the default, weak app_shutdown, which immediately calls the
> shutdown hypercall.  This short circuits the vtpmmgr clean up logic.  We
> need to perform the clean up to actually Flush our key out of the tpm.
> 
> Setting do_shutdown is one step in that direction, but vtpmmgr will most
> likely be waiting in tpmback_req_any.  We need to call shutdown_tpmback
> to cancel the wait inside tpmback and perform the shutdown.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibaut@ens-lyon.org>

> ---
>  stubdom/vtpmmgr/vtpmmgr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/stubdom/vtpmmgr/vtpmmgr.c b/stubdom/vtpmmgr/vtpmmgr.c
> index 9fddaa24f8..46ea018921 100644
> --- a/stubdom/vtpmmgr/vtpmmgr.c
> +++ b/stubdom/vtpmmgr/vtpmmgr.c
> @@ -67,11 +67,21 @@ int hw_is_tpm2(void)
>      return (hardware_version.hw_version == TPM2_HARDWARE) ? 1 : 0;
>  }
>  
> +static int do_shutdown;
> +
> +void app_shutdown(unsigned int reason)
> +{
> +    printk("Shutdown requested: %d\n", reason);
> +    do_shutdown = 1;
> +
> +    shutdown_tpmback();
> +}
> +
>  void main_loop(void) {
>     tpmcmd_t* tpmcmd;
>     uint8_t respbuf[TCPA_MAX_BUFFER_LENGTH];
>  
> -   while(1) {
> +   while (!do_shutdown) {
>        /* Wait for requests from a vtpm */
>        vtpmloginfo(VTPM_LOG_VTPM, "Waiting for commands from vTPM's:\n");
>        if((tpmcmd = tpmback_req_any()) == NULL) {
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0
  2021-05-04 12:48 ` [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0 Jason Andryuk
@ 2021-05-04 13:33   ` Samuel Thibault
  2021-05-04 17:23     ` Jason Andryuk
  0 siblings, 1 reply; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 13:33 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 08:48:42 -0400, a ecrit:
> GetRandom passthrough currently fails when using vtpmmgr with a hardware
> TPM 2.0.
> vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom
> vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30)
> 
> When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2
> TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command.  Besides the
> differing ordinal, the TPM 1.2 uses 32bit sizes for the request and
> response (vs. 16bit for TPM2).
> 
> Place the random output directly into the tpmcmd->resp and build the
> packet around it.  This avoids bouncing through an extra buffer, but the
> header has to be written after grabbing the random bytes so we have the
> number of bytes to include in the size.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  stubdom/vtpmmgr/marshal.h          | 10 +++++++
>  stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h
> index dce19c6439..20da22af09 100644
> --- a/stubdom/vtpmmgr/marshal.h
> +++ b/stubdom/vtpmmgr/marshal.h
> @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) {
>  	return rv;
>  }
>  
> +static
> +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) {
> +	int rv = 0;
> +	rv += sizeof_UINT16(ptr);
> +	rv += sizeof_UINT32(ptr);
> +	rv += sizeof_UINT32(ptr);
> +	return rv;
> +}
> +
>  static
>  inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr,
>  		TPM_TAG tag,
> @@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max,
>  #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r)
>  #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r)
>  #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r)
> +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p)
>  
>  #endif
> diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> index 2ac14fae77..7ca1d9df94 100644
> --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c
> +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> @@ -47,6 +47,7 @@
>  #include "vtpm_disk.h"
>  #include "vtpmmgr.h"
>  #include "tpm.h"
> +#include "tpm2.h"
>  #include "tpmrsa.h"
>  #include "tcg.h"
>  #include "mgmt_authority.h"
> @@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq)
>  	return 1;
>  }
>  
> +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque,
> +				    tpmcmd_t* tpmcmd)
> +{
> +	TPM_RESULT status = TPM_SUCCESS;
> +	TPM_TAG tag;
> +	UINT32 size;
> +	UINT32 rand_offset;
> +	UINT32 rand_size;
> +	TPM_COMMAND_CODE ord;
> +	BYTE *p;
> +
> +	p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord);
> +
> +	if (!hw_is_tpm2()) {
> +		size = TCPA_MAX_BUFFER_LENGTH;
> +		TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len,
> +					      tpmcmd->resp, &size));
> +		tpmcmd->resp_len = size;
> +
> +		return TPM_SUCCESS;
> +	}


We need to check for the size of the request before unpacking (which
doesn't check for it), don't we?

> +	/* TPM_GetRandom req: <header><uint32 num bytes> */
> +	unpack_UINT32(p, &rand_size);
> +
> +	/* Call TPM2_GetRandom but return a TPM_GetRandom response. */
> +	/* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */
> +        rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) +
> +		      sizeof_UINT32(tpmcmd->resp);

There is a spurious indentation here, at first sight I even thought it
was part of the comment.


We also need to check that rand_size is not too large?
- that the returned data won't overflow tpmcmd->resp + rand_offset
- that it fits in a UINT16

Also, TPM2_GetRandom casts bytesRequested into UINT16*, that's bogus, it
should use a local UINT16 variable and assign *bytesRequested.

> +	TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset));
> +
> +	p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND,
> +				rand_offset + rand_size, status);
> +	p = pack_UINT32(p, rand_size);
> +	tpmcmd->resp_len = rand_offset + rand_size;
> +
> +	return status;
> +
> +abort_egress:
> +	tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE;
> +	pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status);
> +
> +	return status;
> +}
> +
>  TPM_RESULT vtpmmgr_handle_cmd(
>  		struct tpm_opaque *opaque,
>  		tpmcmd_t* tpmcmd)
> @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd(
>  		switch(ord) {
>  		case TPM_ORD_GetRandom:
>  			vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n");
> +			return vtpmmgr_handle_getrandom(opaque, tpmcmd);
>  			break;

Drop the break, then. I would say also move (or drop) the log, like the
other cases.

>  		case TPM_ORD_PcrRead:
>  			vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n");
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 13:13   ` Samuel Thibault
@ 2021-05-04 17:04     ` Jason Andryuk
  2021-05-04 17:07       ` Samuel Thibault
  0 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 17:04 UTC (permalink / raw)
  To: Samuel Thibault, Jason Andryuk, xen-devel, Ian Jackson, Wei Liu,
	Daniel De Graaf, Quan Xu

On Tue, May 4, 2021 at 9:13 AM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Jason Andryuk, le mar. 04 mai 2021 08:48:37 -0400, a ecrit:
> > Bypass taking ownership of the TPM2 if an srk_handle is specified.
> >
> > This srk_handle must be usable with Null auth for the time being.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
> >  docs/man/xen-vtpmmgr.7.pod |  7 +++++++
> >  stubdom/vtpmmgr/init.c     | 11 ++++++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod
> > index 875dcce508..3286954568 100644
> > --- a/docs/man/xen-vtpmmgr.7.pod
> > +++ b/docs/man/xen-vtpmmgr.7.pod
> > @@ -92,6 +92,13 @@ Valid arguments:
> >
> >  =over 4
> >
> > +=item srk_handle=<HANDLE>
>
> Is this actually srk_handle= or srk_handle: ?

Whoops.  It's srk_handle: .  I just copy and pasted here.

> The code tests for the latter. The problem seems to "exist" also for
> owner_auth: and srk_auth: but both = and : work actually because strncmp
> is told not to check for = and :

owner_auth & srk_auth don't check :, but then they don't skip : or =
when passing the string to parse_auth_string.  So they can't work
properly?

srk_handle: does check for that entire string.

> We'd better clean this up to avoid confusions.

Right, so what do we want?  I'm leaning toward standardizing on =
since the tpm.*= options look to parse properly.  Given : doesn't seem
like it could work, we don't need to attempt to maintain backwards
compatibility.

Thanks for the review.

-Jason


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/9] vtpmmgr: Flush all transient keys
  2021-05-04 13:16   ` Samuel Thibault
@ 2021-05-04 17:05     ` Jason Andryuk
  2021-05-04 17:07       ` Samuel Thibault
  0 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 17:05 UTC (permalink / raw)
  To: Samuel Thibault, Jason Andryuk, xen-devel, Daniel De Graaf, Quan Xu

On Tue, May 4, 2021 at 9:16 AM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Jason Andryuk, le mar. 04 mai 2021 08:48:40 -0400, a ecrit:
> > We're only flushing 2 transients, but there are 3 handles.  Use <= to also
> > flush the third handle.
> >
> > The number of transient handles/keys is hardware dependent, so this
> > should query for the limit.  And assignment of handles is assumed to be
> > sequential from the minimum.  That may not be guaranteed, but seems okay
> > with my tpm2.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>
> Maybe explicit in the log that TRANSIENT_LAST is actually inclusive?

In the commit message?  Sounds good to me.

> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks,
Jason


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 17:04     ` Jason Andryuk
@ 2021-05-04 17:07       ` Samuel Thibault
  2021-05-04 17:27         ` Jason Andryuk
  0 siblings, 1 reply; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 17:07 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 13:04:47 -0400, a ecrit:
> owner_auth & srk_auth don't check :, but then they don't skip : or =
> when passing the string to parse_auth_string.  So they can't work
> properly?

They happen to "work" just because there is no other parameter prefixed
the same.

> > We'd better clean this up to avoid confusions.
> 
> Right, so what do we want?  I'm leaning toward standardizing on =
> since the tpm.*= options look to parse properly.

I'd say so too. Also because that's what is apparently documented.

Samuel


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/9] vtpmmgr: Flush all transient keys
  2021-05-04 17:05     ` Jason Andryuk
@ 2021-05-04 17:07       ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 17:07 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 13:05:33 -0400, a ecrit:
> On Tue, May 4, 2021 at 9:16 AM Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> >
> > Jason Andryuk, le mar. 04 mai 2021 08:48:40 -0400, a ecrit:
> > > We're only flushing 2 transients, but there are 3 handles.  Use <= to also
> > > flush the third handle.
> > >
> > > The number of transient handles/keys is hardware dependent, so this
> > > should query for the limit.  And assignment of handles is assumed to be
> > > sequential from the minimum.  That may not be guaranteed, but seems okay
> > > with my tpm2.
> > >
> > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> >
> > Maybe explicit in the log that TRANSIENT_LAST is actually inclusive?
> 
> In the commit message?  Sounds good to me.

Yes, please.

Samuel


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0
  2021-05-04 13:33   ` Samuel Thibault
@ 2021-05-04 17:23     ` Jason Andryuk
  2021-05-04 17:47       ` Samuel Thibault
  0 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 17:23 UTC (permalink / raw)
  To: Samuel Thibault, Jason Andryuk, xen-devel, Daniel De Graaf, Quan Xu

On Tue, May 4, 2021 at 9:33 AM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Jason Andryuk, le mar. 04 mai 2021 08:48:42 -0400, a ecrit:
> > GetRandom passthrough currently fails when using vtpmmgr with a hardware
> > TPM 2.0.
> > vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom
> > vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30)
> >
> > When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2
> > TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command.  Besides the
> > differing ordinal, the TPM 1.2 uses 32bit sizes for the request and
> > response (vs. 16bit for TPM2).
> >
> > Place the random output directly into the tpmcmd->resp and build the
> > packet around it.  This avoids bouncing through an extra buffer, but the
> > header has to be written after grabbing the random bytes so we have the
> > number of bytes to include in the size.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
> >  stubdom/vtpmmgr/marshal.h          | 10 +++++++
> >  stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h
> > index dce19c6439..20da22af09 100644
> > --- a/stubdom/vtpmmgr/marshal.h
> > +++ b/stubdom/vtpmmgr/marshal.h
> > @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) {
> >       return rv;
> >  }
> >
> > +static
> > +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) {
> > +     int rv = 0;
> > +     rv += sizeof_UINT16(ptr);
> > +     rv += sizeof_UINT32(ptr);
> > +     rv += sizeof_UINT32(ptr);
> > +     return rv;
> > +}
> > +
> >  static
> >  inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr,
> >               TPM_TAG tag,
> > @@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max,
> >  #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r)
> >  #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r)
> >  #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r)
> > +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p)
> >
> >  #endif
> > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> > index 2ac14fae77..7ca1d9df94 100644
> > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c
> > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c
> > @@ -47,6 +47,7 @@
> >  #include "vtpm_disk.h"
> >  #include "vtpmmgr.h"
> >  #include "tpm.h"
> > +#include "tpm2.h"
> >  #include "tpmrsa.h"
> >  #include "tcg.h"
> >  #include "mgmt_authority.h"
> > @@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq)
> >       return 1;
> >  }
> >
> > +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque,
> > +                                 tpmcmd_t* tpmcmd)
> > +{
> > +     TPM_RESULT status = TPM_SUCCESS;
> > +     TPM_TAG tag;
> > +     UINT32 size;
> > +     UINT32 rand_offset;
> > +     UINT32 rand_size;
> > +     TPM_COMMAND_CODE ord;
> > +     BYTE *p;
> > +
> > +     p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord);
> > +
> > +     if (!hw_is_tpm2()) {
> > +             size = TCPA_MAX_BUFFER_LENGTH;
> > +             TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len,
> > +                                           tpmcmd->resp, &size));
> > +             tpmcmd->resp_len = size;
> > +
> > +             return TPM_SUCCESS;
> > +     }
>
>
> We need to check for the size of the request before unpacking (which
> doesn't check for it), don't we?

Yes, good catch.  vtpmmgr_handle_cmd doesn't check either.

> > +     /* TPM_GetRandom req: <header><uint32 num bytes> */
> > +     unpack_UINT32(p, &rand_size);
> > +
> > +     /* Call TPM2_GetRandom but return a TPM_GetRandom response. */
> > +     /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */
> > +        rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) +
> > +                   sizeof_UINT32(tpmcmd->resp);
>
> There is a spurious indentation here, at first sight I even thought it
> was part of the comment.

Sorry about that - it was 8 spaces instead of a tab.

> We also need to check that rand_size is not too large?
> - that the returned data won't overflow tpmcmd->resp + rand_offset
> - that it fits in a UINT16

Yes, will do.

> Also, TPM2_GetRandom casts bytesRequested into UINT16*, that's bogus, it
> should use a local UINT16 variable and assign *bytesRequested.

Good catch.  I'll do that in a new patch.

> > +     TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset));
> > +
> > +     p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND,
> > +                             rand_offset + rand_size, status);
> > +     p = pack_UINT32(p, rand_size);
> > +     tpmcmd->resp_len = rand_offset + rand_size;
> > +
> > +     return status;
> > +
> > +abort_egress:
> > +     tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE;
> > +     pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status);
> > +
> > +     return status;
> > +}
> > +
> >  TPM_RESULT vtpmmgr_handle_cmd(
> >               struct tpm_opaque *opaque,
> >               tpmcmd_t* tpmcmd)
> > @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd(
> >               switch(ord) {
> >               case TPM_ORD_GetRandom:
> >                       vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n");
> > +                     return vtpmmgr_handle_getrandom(opaque, tpmcmd);
> >                       break;
>
> Drop the break, then. I would say also move (or drop) the log, like the
> other cases.

Will drop the break.  I would just leave the log since it matches the
other cases in this case statement.  But I can remove it if you want.

Regards,
Jason


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 17:07       ` Samuel Thibault
@ 2021-05-04 17:27         ` Jason Andryuk
  2021-05-04 17:48           ` Samuel Thibault
  0 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2021-05-04 17:27 UTC (permalink / raw)
  To: Samuel Thibault, xen-devel, Ian Jackson, Wei Liu,
	Daniel De Graaf, Quan Xu

On Tue, May 4, 2021 at 1:07 PM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Jason Andryuk, le mar. 04 mai 2021 13:04:47 -0400, a ecrit:
> > owner_auth & srk_auth don't check :, but then they don't skip : or =
> > when passing the string to parse_auth_string.  So they can't work
> > properly?
>
> They happen to "work" just because there is no other parameter prefixed
> the same.

parse_auth_string fails on the ":".

Just tested "owner_auth:well-known"
ERROR[VTPM]: Invalid auth string :well-known
ERROR[VTPM]: Invalid Option owner_auth:well-known
ERROR[VTPM]: Command line parsing failed! exiting..

> > > We'd better clean this up to avoid confusions.
> >
> > Right, so what do we want?  I'm leaning toward standardizing on =
> > since the tpm.*= options look to parse properly.
>
> I'd say so too. Also because that's what is apparently documented.

Ok, thanks.

Regards,
Jason


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0
  2021-05-04 17:23     ` Jason Andryuk
@ 2021-05-04 17:47       ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 17:47 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 13:23:57 -0400, a ecrit:
> > > @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd(
> > >               switch(ord) {
> > >               case TPM_ORD_GetRandom:
> > >                       vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n");
> > > +                     return vtpmmgr_handle_getrandom(opaque, tpmcmd);
> > >                       break;
> >
> > Drop the break, then. I would say also move (or drop) the log, like the
> > other cases.
> 
> Will drop the break.  I would just leave the log since it matches the
> other cases in this case statement.

Mmm, right these are all pass-through cases so better log them the same.

Samuel


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2
  2021-05-04 17:27         ` Jason Andryuk
@ 2021-05-04 17:48           ` Samuel Thibault
  0 siblings, 0 replies; 30+ messages in thread
From: Samuel Thibault @ 2021-05-04 17:48 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Wei Liu, Daniel De Graaf, Quan Xu

Jason Andryuk, le mar. 04 mai 2021 13:27:36 -0400, a ecrit:
> On Tue, May 4, 2021 at 1:07 PM Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> >
> > Jason Andryuk, le mar. 04 mai 2021 13:04:47 -0400, a ecrit:
> > > owner_auth & srk_auth don't check :, but then they don't skip : or =
> > > when passing the string to parse_auth_string.  So they can't work
> > > properly?
> >
> > They happen to "work" just because there is no other parameter prefixed
> > the same.
> 
> parse_auth_string fails on the ":".
> 
> Just tested "owner_auth:well-known"

owner_auth happens to have the proper size, but srk_auth doesn't.

Samuel


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support
  2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
@ 2021-05-04 17:55   ` Andrew Cooper
  2021-05-07 15:31   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2021-05-04 17:55 UTC (permalink / raw)
  To: Jason Andryuk, xen-devel; +Cc: Ian Jackson, Wei Liu

On 04/05/2021 13:48, Jason Andryuk wrote:
> The vtpmmgr TPM 2.0 support is incomplete.  Add a warning about that to
> the documentation so others don't have to work through discovering it is
> broken.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is definitely the kind of health warning needed for people playing
this area.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support
  2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
  2021-05-04 17:55   ` Andrew Cooper
@ 2021-05-07 15:31   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2021-05-07 15:31 UTC (permalink / raw)
  To: Jason Andryuk, xen-devel; +Cc: Ian Jackson, Wei Liu

On 5/4/21 8:48 AM, Jason Andryuk wrote:
> The vtpmmgr TPM 2.0 support is incomplete.  Add a warning about that to
> the documentation so others don't have to work through discovering it is
> broken.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

>  docs/man/xen-vtpmmgr.7.pod | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/docs/man/xen-vtpmmgr.7.pod b/docs/man/xen-vtpmmgr.7.pod
> index af825a7ffe..875dcce508 100644
> --- a/docs/man/xen-vtpmmgr.7.pod
> +++ b/docs/man/xen-vtpmmgr.7.pod
> @@ -222,6 +222,17 @@ XSM label, not the kernel.
>  
>  =head1 Appendix B: vtpmmgr on TPM 2.0
>  
> +=head2 WARNING: Incomplete - cannot persist data
> +
> +TPM 2.0 support for vTPM manager is incomplete.  There is no support for
> +persisting an encryption key, so vTPM manager regenerates primary and secondary
> +key handles each boot.
> +
> +Also, the vTPM manger group command implementation hardcodes TPM 1.2 commands.
> +This means running manage-vtpmmgr.pl fails when the TPM 2.0 hardware rejects
> +the TPM 1.2 commands.  vTPM manager with TPM 2.0 cannot create groups and
> +therefore cannot persist vTPM contents.
> +
>  =head2 Manager disk image setup:
>  
>  The vTPM Manager requires a disk image to store its encrypted data. The image
> 



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/9] vtpmmgr: Print error code to aid debugging
  2021-05-04 12:48 ` [PATCH 2/9] vtpmmgr: Print error code to aid debugging Jason Andryuk
  2021-05-04 13:03   ` Samuel Thibault
@ 2021-05-07 15:33   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2021-05-07 15:33 UTC (permalink / raw)
  To: Jason Andryuk, xen-devel; +Cc: Daniel De Graaf, Quan Xu, Samuel Thibault

On 5/4/21 8:48 AM, Jason Andryuk wrote:
> tpm_get_error_name returns "Unknown Error Code" when an error string
> is not defined.  In that case, we should print the Error Code so it can
> be looked up offline.  tpm_get_error_name returns a const string, so
> just have the two callers always print the error code so it is always
> available.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

>  stubdom/vtpmmgr/tpm.c  | 2 +-
>  stubdom/vtpmmgr/tpm2.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/stubdom/vtpmmgr/tpm.c b/stubdom/vtpmmgr/tpm.c
> index 779cddd64e..83b2bc16b2 100644
> --- a/stubdom/vtpmmgr/tpm.c
> +++ b/stubdom/vtpmmgr/tpm.c
> @@ -109,7 +109,7 @@
>  			UINT32 rsp_status; \
>  			UNPACK_OUT(TPM_RSP_HEADER, &rsp_tag, &rsp_len, &rsp_status); \
>  			if (rsp_status != TPM_SUCCESS) { \
> -				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(rsp_status)); \
> +				vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(rsp_status), rsp_status); \
>  				status = rsp_status; \
>  				goto abort_egress; \
>  			} \
> diff --git a/stubdom/vtpmmgr/tpm2.c b/stubdom/vtpmmgr/tpm2.c
> index c9f1016ab5..655e6d164c 100644
> --- a/stubdom/vtpmmgr/tpm2.c
> +++ b/stubdom/vtpmmgr/tpm2.c
> @@ -126,7 +126,7 @@
>      ptr = unpack_TPM_RSP_HEADER(ptr, \
>            &(tag), &(paramSize), &(status));\
>      if ((status) != TPM_SUCCESS){ \
> -        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s\n", tpm_get_error_name(status));\
> +        vtpmlogerror(VTPM_LOG_TPM, "Failed with return code %s (%x)\n", tpm_get_error_name(status), (status));\
>          goto abort_egress;\
>      }\
>  } while(0)
> 



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/9] stubom: newlib: Enable C99 formats for %z
  2021-05-04 12:48 ` [PATCH 3/9] stubom: newlib: Enable C99 formats for %z Jason Andryuk
  2021-05-04 13:08   ` Samuel Thibault
@ 2021-05-07 15:37   ` Daniel P. Smith
  1 sibling, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2021-05-07 15:37 UTC (permalink / raw)
  To: Jason Andryuk, xen-devel; +Cc: Ian Jackson, Wei Liu, Samuel Thibault

On 5/4/21 8:48 AM, Jason Andryuk wrote:
> vtpmmgr was changed to print size_t with the %z modifier, but newlib
> isn't compiled with %z support.  So you get output like:
> 
> root seal: zu; sector of 13: zu
> root: zu v=zu
> itree: 36; sector of 112: zu
> group: zu v=zu id=zu md=zu
> group seal: zu; 5 in parent: zu; sector of 13: zu
> vtpm: zu+zu; sector of 48: zu
> 
> Enable the C99 formats in newlib so vtpmmgr prints the numeric values.
> 
> Fixes 9379af08ccc0 "stubdom: vtpmmgr: Correctly format size_t with %z
> when printing."
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

> I haven't tried, but the other option would be to cast size_t and avoid
> %z.  Since this seems to be the only mini-os use of %z, that may be
> better than building a larger newlib.
> ---
>  stubdom/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/stubdom/Makefile b/stubdom/Makefile
> index 90d9ffcd9f..c6de5f68ae 100644
> --- a/stubdom/Makefile
> +++ b/stubdom/Makefile
> @@ -105,7 +105,7 @@ cross-newlib: $(NEWLIB_STAMPFILE)
>  $(NEWLIB_STAMPFILE): mk-headers-$(XEN_TARGET_ARCH) newlib-$(NEWLIB_VERSION)
>  	mkdir -p newlib-$(XEN_TARGET_ARCH)
>  	( cd newlib-$(XEN_TARGET_ARCH) && \
> -	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --disable-multilib && \
> +	  CC_FOR_TARGET="$(CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(NEWLIB_CFLAGS)" AR_FOR_TARGET=$(AR) LD_FOR_TARGET=$(LD) RANLIB_FOR_TARGET=$(RANLIB) ../newlib-$(NEWLIB_VERSION)/configure --prefix=$(CROSS_PREFIX) --verbose --target=$(GNU_TARGET_ARCH)-xen-elf --enable-newlib-io-long-long --enable-newlib-io-c99-formats --disable-multilib && \
>  	  $(MAKE) DESTDIR= && \
>  	  $(MAKE) DESTDIR= install )
>  
> 



^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-05-07 15:37 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 12:48 [PATCH 0/9] vtpmmgr: Some fixes - still incomplete Jason Andryuk
2021-05-04 12:48 ` [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support Jason Andryuk
2021-05-04 17:55   ` Andrew Cooper
2021-05-07 15:31   ` Daniel P. Smith
2021-05-04 12:48 ` [PATCH 2/9] vtpmmgr: Print error code to aid debugging Jason Andryuk
2021-05-04 13:03   ` Samuel Thibault
2021-05-07 15:33   ` Daniel P. Smith
2021-05-04 12:48 ` [PATCH 3/9] stubom: newlib: Enable C99 formats for %z Jason Andryuk
2021-05-04 13:08   ` Samuel Thibault
2021-05-07 15:37   ` Daniel P. Smith
2021-05-04 12:48 ` [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2 Jason Andryuk
2021-05-04 13:13   ` Samuel Thibault
2021-05-04 17:04     ` Jason Andryuk
2021-05-04 17:07       ` Samuel Thibault
2021-05-04 17:27         ` Jason Andryuk
2021-05-04 17:48           ` Samuel Thibault
2021-05-04 12:48 ` [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown Jason Andryuk
2021-05-04 13:14   ` Samuel Thibault
2021-05-04 12:48 ` [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown Jason Andryuk
2021-05-04 13:15   ` Samuel Thibault
2021-05-04 12:48 ` [PATCH 7/9] vtpmmgr: Flush all transient keys Jason Andryuk
2021-05-04 13:16   ` Samuel Thibault
2021-05-04 17:05     ` Jason Andryuk
2021-05-04 17:07       ` Samuel Thibault
2021-05-04 12:48 ` [PATCH 8/9] vtpmmgr: Shutdown more gracefully Jason Andryuk
2021-05-04 13:18   ` Samuel Thibault
2021-05-04 12:48 ` [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0 Jason Andryuk
2021-05-04 13:33   ` Samuel Thibault
2021-05-04 17:23     ` Jason Andryuk
2021-05-04 17:47       ` Samuel Thibault

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).