[PATCH 0/9] vtpmmgr: Some fixes - still incomplete

[PATCH 0/9] vtpmmgr: Some fixes - still incomplete

[Jason Andryuk]

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

[PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support

[Jason Andryuk]

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

[PATCH 2/9] vtpmmgr: Print error code to aid debugging

[Jason Andryuk]

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

[PATCH 3/9] stubom: newlib: Enable C99 formats for %z

[Jason Andryuk]

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

[PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Jason Andryuk]

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

[PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown

[Jason Andryuk]

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

[PATCH 6/9] vtpmmgr: Flush transient keys on shutdown

[Jason Andryuk]

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

[PATCH 7/9] vtpmmgr: Flush all transient keys

[Jason Andryuk]

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

[PATCH 8/9] vtpmmgr: Shutdown more gracefully

[Jason Andryuk]

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

[PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0

[Jason Andryuk]

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

Re: [PATCH 2/9] vtpmmgr: Print error code to aid debugging

[Samuel Thibault]

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
> 

Re: [PATCH 3/9] stubom: newlib: Enable C99 formats for %z

[Samuel Thibault]

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
> 

Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Samuel Thibault]

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
> 

Re: [PATCH 5/9] vtpmmgr: Move vtpmmgr_shutdown

[Samuel Thibault]

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
> 

Re: [PATCH 6/9] vtpmmgr: Flush transient keys on shutdown

[Samuel Thibault]

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
> 

Re: [PATCH 7/9] vtpmmgr: Flush all transient keys

[Samuel Thibault]

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
> 

Re: [PATCH 8/9] vtpmmgr: Shutdown more gracefully

[Samuel Thibault]

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
> 

Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0

[Samuel Thibault]

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
> 

Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Jason Andryuk]

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

Re: [PATCH 7/9] vtpmmgr: Flush all transient keys

[Jason Andryuk]

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

Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Samuel Thibault]

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

Re: [PATCH 7/9] vtpmmgr: Flush all transient keys

[Samuel Thibault]

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

Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0

[Jason Andryuk]

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

Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Jason Andryuk]

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

Re: [PATCH 9/9] vtpmmgr: Support GetRandom passthrough on TPM 2.0

[Samuel Thibault]

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

Re: [PATCH 4/9] vtpmmgr: Allow specifying srk_handle for TPM2

[Samuel Thibault]

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

Re: [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support

[Andrew Cooper]

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.

Re: [PATCH 1/9] docs: Warn about incomplete vtpmmgr TPM 2.0 support

[Daniel P. Smith]

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
> 

Re: [PATCH 2/9] vtpmmgr: Print error code to aid debugging

[Daniel P. Smith]

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)
> 

Re: [PATCH 3/9] stubom: newlib: Enable C99 formats for %z

[Daniel P. Smith]

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 )
>  
>