[PATCH v2 0/7] tpm: Various minor fixes and enhancements

[PATCH v2 0/7] tpm: Various minor fixes and enhancements

[Simon Glass]

This series contains some minor enhancements for the TPM code to make it
work with Chromium OS verified boot.

Changes in v2:
- Drop limits on the TPM hash size
- Update commit message
- Use constants instead of open-coded values

Simon Glass (7):
  tpm: Require a digest source when extending the PCR
  tpm: Correct the permissions command in TPMv1
  tpm: Correct the define-space command in TPMv2
  tpm: sandbox: Allow init of TPM in a different phase
  tpm: Allow reporting the internal state
  tpm: Implement state command for Cr50
  tpm: Allow committing non-volatile data

 cmd/tpm-common.c               |  20 ++++++
 cmd/tpm-user-utils.h           |   2 +
 cmd/tpm-v1.c                   |   3 +-
 cmd/tpm-v2.c                   |   3 +
 cmd/tpm_test.c                 |   5 +-
 drivers/tpm/cr50_i2c.c         | 117 +++++++++++++++++++++++++++++++++
 drivers/tpm/tpm-uclass.c       |  10 +++
 drivers/tpm/tpm2_tis_sandbox.c |  17 ++++-
 include/tpm-common.h           |  20 ++++++
 include/tpm-v2.h               |  68 +++++++++++++++++++
 include/tpm_api.h              |   8 ++-
 lib/tpm-v1.c                   |   5 +-
 lib/tpm-v2.c                   |  68 +++++++++++++++++--
 lib/tpm_api.c                  |  10 +--
 test/dm/Makefile               |   1 +
 test/dm/tpm.c                  |  34 ++++++++++
 16 files changed, 370 insertions(+), 21 deletions(-)
 create mode 100644 test/dm/tpm.c

-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 1/7] tpm: Require a digest source when extending the PCR

[Simon Glass]

This feature is used for measured boot, so we can add a log entry to the
TCPA with some information about where the digest comes from. It is not
currently supported in the TPM drivers, but add it to the API so that
code which expects it can signal its request.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Drop limits on the TPM hash size
- Update commit message

 cmd/tpm-v1.c      |  3 ++-
 cmd/tpm_test.c    |  5 +++--
 include/tpm_api.h |  8 +++++---
 lib/tpm-v2.c      |  2 ++
 lib/tpm_api.c     | 10 ++++++----
 5 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c
index bf238a9f2e3..0869b707757 100644
--- a/cmd/tpm-v1.c
+++ b/cmd/tpm-v1.c
@@ -131,7 +131,8 @@ static int do_tpm_extend(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_FAILURE;
 	}
 
-	rc = tpm_pcr_extend(dev, index, in_digest, out_digest);
+	rc = tpm_pcr_extend(dev, index, in_digest, sizeof(in_digest),
+			    out_digest, "test");
 	if (!rc) {
 		puts("PCR value after execution of the command:\n");
 		print_byte_string(out_digest, sizeof(out_digest));
diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c
index a3ccb12f53a..b35eae81dc3 100644
--- a/cmd/tpm_test.c
+++ b/cmd/tpm_test.c
@@ -91,7 +91,8 @@ static int test_early_extend(struct udevice *dev)
 	tpm_init(dev);
 	TPM_CHECK(tpm_startup(dev, TPM_ST_CLEAR));
 	TPM_CHECK(tpm_continue_self_test(dev));
-	TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, value_out));
+	TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, sizeof(value_in), value_out,
+				 "test"));
 	printf("done\n");
 	return 0;
 }
@@ -438,7 +439,7 @@ static int test_timing(struct udevice *dev)
 		   100);
 	TTPM_CHECK(tpm_nv_read_value(dev, INDEX0, (uint8_t *)&x, sizeof(x)),
 		   100);
-	TTPM_CHECK(tpm_pcr_extend(dev, 0, in, out), 200);
+	TTPM_CHECK(tpm_pcr_extend(dev, 0, in, sizeof(in), out, "test"), 200);
 	TTPM_CHECK(tpm_set_global_lock(dev), 50);
 	TTPM_CHECK(tpm_tsc_physical_presence(dev, PHYS_PRESENCE), 100);
 	printf("done\n");
diff --git a/include/tpm_api.h b/include/tpm_api.h
index 11aa14eb793..3c8e48bc255 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -81,14 +81,16 @@ u32 tpm_nv_write_value(struct udevice *dev, u32 index, const void *data,
  *
  * @param dev		TPM device
  * @param index		index of the PCR
- * @param in_digest	160-bit value representing the event to be
+ * @param in_digest	160/256-bit value representing the event to be
  *			recorded
- * @param out_digest	160-bit PCR value after execution of the
+ * @param size		size of digest in bytes
+ * @param out_digest	160/256-bit PCR value after execution of the
  *			command
+ * @param name		additional info about where the digest comes from
  * Return: return code of the operation
  */
 u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
-		   void *out_digest);
+		   uint size, void *out_digest, const char *name);
 
 /**
  * Issue a TPM_PCRRead command.
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 1bf627853af..6058f2e1e4f 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -157,6 +157,8 @@ u32 tpm2_pcr_extend(struct udevice *dev, u32 index, u32 algorithm,
 	};
 	int ret;
 
+	if (!digest)
+		return -EINVAL;
 	/*
 	 * Fill the command structure starting from the first buffer:
 	 *     - the digest
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 032f383ca04..aa4a9fd406c 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -140,15 +140,17 @@ u32 tpm_write_lock(struct udevice *dev, u32 index)
 }
 
 u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
-		   void *out_digest)
+		   uint size, void *out_digest, const char *name)
 {
-	if (tpm_is_v1(dev))
+	if (tpm_is_v1(dev)) {
 		return tpm1_extend(dev, index, in_digest, out_digest);
-	else if (tpm_is_v2(dev))
+	} else if (tpm_is_v2(dev)) {
 		return tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, in_digest,
 				       TPM2_DIGEST_LEN);
-	else
+		/* @name is ignored as we do not support measured boot */
+	} else {
 		return -ENOSYS;
+	}
 }
 
 u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 1/7] tpm: Require a digest source when extending the PCR

[Heinrich Schuchardt]

On 8/13/22 21:56, Simon Glass wrote:
> This feature is used for measured boot, so we can add a log entry to the
> TCPA with some information about where the digest comes from. It is not
> currently supported in the TPM drivers, but add it to the API so that
> code which expects it can signal its request.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Drop limits on the TPM hash size
> - Update commit message
>
>   cmd/tpm-v1.c      |  3 ++-
>   cmd/tpm_test.c    |  5 +++--
>   include/tpm_api.h |  8 +++++---
>   lib/tpm-v2.c      |  2 ++
>   lib/tpm_api.c     | 10 ++++++----
>   5 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c
> index bf238a9f2e3..0869b707757 100644
> --- a/cmd/tpm-v1.c
> +++ b/cmd/tpm-v1.c
> @@ -131,7 +131,8 @@ static int do_tpm_extend(struct cmd_tbl *cmdtp, int flag, int argc,
>   		return CMD_RET_FAILURE;
>   	}
>
> -	rc = tpm_pcr_extend(dev, index, in_digest, out_digest);
> +	rc = tpm_pcr_extend(dev, index, in_digest, sizeof(in_digest),
> +			    out_digest, "test");

The value "test" seems inadequate in this context. Should this be a
command line argument? Or how about "cmd".

>   	if (!rc) {
>   		puts("PCR value after execution of the command:\n");
>   		print_byte_string(out_digest, sizeof(out_digest));
> diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c
> index a3ccb12f53a..b35eae81dc3 100644
> --- a/cmd/tpm_test.c
> +++ b/cmd/tpm_test.c
> @@ -91,7 +91,8 @@ static int test_early_extend(struct udevice *dev)
>   	tpm_init(dev);
>   	TPM_CHECK(tpm_startup(dev, TPM_ST_CLEAR));
>   	TPM_CHECK(tpm_continue_self_test(dev));
> -	TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, value_out));
> +	TPM_CHECK(tpm_pcr_extend(dev, 1, value_in, sizeof(value_in), value_out,
> +				 "test"));

Here the string "test" is adequate.

>   	printf("done\n");
>   	return 0;
>   }
> @@ -438,7 +439,7 @@ static int test_timing(struct udevice *dev)
>   		   100);
>   	TTPM_CHECK(tpm_nv_read_value(dev, INDEX0, (uint8_t *)&x, sizeof(x)),
>   		   100);
> -	TTPM_CHECK(tpm_pcr_extend(dev, 0, in, out), 200);
> +	TTPM_CHECK(tpm_pcr_extend(dev, 0, in, sizeof(in), out, "test"), 200);
>   	TTPM_CHECK(tpm_set_global_lock(dev), 50);
>   	TTPM_CHECK(tpm_tsc_physical_presence(dev, PHYS_PRESENCE), 100);
>   	printf("done\n");
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 11aa14eb793..3c8e48bc255 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -81,14 +81,16 @@ u32 tpm_nv_write_value(struct udevice *dev, u32 index, const void *data,
>    *
>    * @param dev		TPM device
>    * @param index		index of the PCR
> - * @param in_digest	160-bit value representing the event to be
> + * @param in_digest	160/256-bit value representing the event to be
>    *			recorded
> - * @param out_digest	160-bit PCR value after execution of the
> + * @param size		size of digest in bytes
> + * @param out_digest	160/256-bit PCR value after execution of the
>    *			command
> + * @param name		additional info about where the digest comes from

This does not indicate the usage and if the argument will be stored on
the TPM. I would prefer:

"digest source used for log output"

Best regards

Heinrich

>    * Return: return code of the operation
>    */
>   u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
> -		   void *out_digest);
> +		   uint size, void *out_digest, const char *name);
>
>   /**
>    * Issue a TPM_PCRRead command.
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853af..6058f2e1e4f 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -157,6 +157,8 @@ u32 tpm2_pcr_extend(struct udevice *dev, u32 index, u32 algorithm,
>   	};
>   	int ret;
>
> +	if (!digest)
> +		return -EINVAL;
>   	/*
>   	 * Fill the command structure starting from the first buffer:
>   	 *     - the digest
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 032f383ca04..aa4a9fd406c 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -140,15 +140,17 @@ u32 tpm_write_lock(struct udevice *dev, u32 index)
>   }
>
>   u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
> -		   void *out_digest)
> +		   uint size, void *out_digest, const char *name)
>   {
> -	if (tpm_is_v1(dev))
> +	if (tpm_is_v1(dev)) {
>   		return tpm1_extend(dev, index, in_digest, out_digest);
> -	else if (tpm_is_v2(dev))
> +	} else if (tpm_is_v2(dev)) {
>   		return tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, in_digest,
>   				       TPM2_DIGEST_LEN);
> -	else
> +		/* @name is ignored as we do not support measured boot */
> +	} else {
>   		return -ENOSYS;
> +	}
>   }
>
>   u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)

[PATCH v2 2/7] tpm: Correct the permissions command in TPMv1

[Simon Glass]

The offset here is incorrect. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 lib/tpm-v1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 22a769c5874..d0e3ab1b21d 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 		0x0, 0x0, 0x0, 0x4,
 	};
 	const size_t index_offset = 18;
-	const size_t perm_offset = 60;
+	const size_t perm_offset = 74;
 	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
 	size_t response_length = sizeof(response);
 	u32 err;
 
-	if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command),
+	if (pack_byte_string(buf, sizeof(buf), "sd",
+			     0, command, sizeof(command),
 			     index_offset, index))
 		return TPM_LIB_ERROR;
 	err = tpm_sendrecv_command(dev, buf, response, &response_length);
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 2/7] tpm: Correct the permissions command in TPMv1

[Ilias Apalodimas]

Hi Simon

On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
>
> The offset here is incorrect. Fix it.

Since we got it wrong the first time, any chance you can give me a
link to the spec describing these offsets (both for this and the
subsequent patch)

Thanks
/Ilias
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  lib/tpm-v1.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c5874..d0e3ab1b21d 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>                 0x0, 0x0, 0x0, 0x4,
>         };
>         const size_t index_offset = 18;
> -       const size_t perm_offset = 60;
> +       const size_t perm_offset = 74;
>         u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
>         size_t response_length = sizeof(response);
>         u32 err;
>
> -       if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command),
> +       if (pack_byte_string(buf, sizeof(buf), "sd",
> +                            0, command, sizeof(command),
>                              index_offset, index))
>                 return TPM_LIB_ERROR;
>         err = tpm_sendrecv_command(dev, buf, response, &response_length);
> --
> 2.37.1.595.g718a3a8f04-goog
>

Re: [PATCH v2 2/7] tpm: Correct the permissions command in TPMv1

[Simon Glass]

Hi Ilias,

On Tue, 16 Aug 2022 at 07:59, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon
>
> On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> >
> > The offset here is incorrect. Fix it.
>
> Since we got it wrong the first time, any chance you can give me a
> link to the spec describing these offsets (both for this and the
> subsequent patch)

Yes I have been using this document:

TPM Main
Part 3 Commands
Specification Version 1.2
Level 2 Revision 116
1 March 2011
TCG Published

(TPM_ORD_GetCapability)

Regards,
Simon


>
> Thanks
> /Ilias
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  lib/tpm-v1.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> > index 22a769c5874..d0e3ab1b21d 100644
> > --- a/lib/tpm-v1.c
> > +++ b/lib/tpm-v1.c
> > @@ -456,12 +456,13 @@ u32 tpm1_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> >                 0x0, 0x0, 0x0, 0x4,
> >         };
> >         const size_t index_offset = 18;
> > -       const size_t perm_offset = 60;
> > +       const size_t perm_offset = 74;
> >         u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> >         size_t response_length = sizeof(response);
> >         u32 err;
> >
> > -       if (pack_byte_string(buf, sizeof(buf), "d", 0, command, sizeof(command),
> > +       if (pack_byte_string(buf, sizeof(buf), "sd",
> > +                            0, command, sizeof(command),
> >                              index_offset, index))
> >                 return TPM_LIB_ERROR;
> >         err = tpm_sendrecv_command(dev, buf, response, &response_length);
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >

[PATCH v2 3/7] tpm: Correct the define-space command in TPMv2

[Simon Glass]

The message format is incorrect. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Use constants instead of open-coded values

 lib/tpm-v2.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 6058f2e1e4f..3e240bb4c67 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -89,14 +89,18 @@ u32 tpm2_nv_define_space(struct udevice *dev, u32 space_index,
 	 * Calculate the offset of the nv_policy piece by adding each of the
 	 * chunks below.
 	 */
-	uint offset = 10 + 8 + 13 + 14;
+	const int platform_len = sizeof(u32);
+	const int session_hdr_len = 13;
+	const int message_len = 14;
+	uint offset = TPM2_HDR_LEN + platform_len + session_hdr_len +
+		message_len;
 	u8 command_v2[COMMAND_BUFFER_SIZE] = {
 		/* header 10 bytes */
 		tpm_u16(TPM2_ST_SESSIONS),	/* TAG */
-		tpm_u32(offset + nv_policy_size),/* Length */
+		tpm_u32(offset + nv_policy_size + 2),/* Length */
 		tpm_u32(TPM2_CC_NV_DEFINE_SPACE),/* Command code */
 
-		/* handles 8 bytes */
+		/* handles 4 bytes */
 		tpm_u32(TPM2_RH_PLATFORM),	/* Primary platform seed */
 
 		/* session header 13 bytes */
@@ -107,12 +111,15 @@ u32 tpm2_nv_define_space(struct udevice *dev, u32 space_index,
 		tpm_u16(0),			/* auth_size */
 
 		/* message 14 bytes + policy */
-		tpm_u16(12 + nv_policy_size),	/* size */
+		tpm_u16(message_len + nv_policy_size),	/* size */
 		tpm_u32(space_index),
 		tpm_u16(TPM2_ALG_SHA256),
 		tpm_u32(nv_attributes),
 		tpm_u16(nv_policy_size),
-		/* nv_policy */
+		/*
+		 * nv_policy
+		 * space_size
+		 */
 	};
 	int ret;
 
@@ -120,8 +127,9 @@ u32 tpm2_nv_define_space(struct udevice *dev, u32 space_index,
 	 * Fill the command structure starting from the first buffer:
 	 *     - the password (if any)
 	 */
-	ret = pack_byte_string(command_v2, sizeof(command_v2), "s",
-			       offset, nv_policy, nv_policy_size);
+	ret = pack_byte_string(command_v2, sizeof(command_v2), "sw",
+			       offset, nv_policy, nv_policy_size,
+			       offset + nv_policy_size, space_size);
 	if (ret)
 		return TPM_LIB_ERROR;
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 4/7] tpm: sandbox: Allow init of TPM in a different phase

[Simon Glass]

At present the emulator assumes that the TPM is inited in the same phase
where it is used. But in fact SPL may init the TPM, so we don't want to
complain when U-Boot proper later uses it. Remove this check.

It might be best to save this information into the device state for the
TPM, so that we can make sure the TPM was inited at some point. For now,
this seems good enough.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 drivers/tpm/tpm2_tis_sandbox.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c
index ac6eb143539..c26f5d35abf 100644
--- a/drivers/tpm/tpm2_tis_sandbox.c
+++ b/drivers/tpm/tpm2_tis_sandbox.c
@@ -366,8 +366,10 @@ static int sandbox_tpm2_check_readyness(struct udevice *dev, int command)
 
 		break;
 	default:
-		if (!tpm->tests_done)
-			return TPM2_RC_NEEDS_TEST;
+		/* Skip this, since the startup may have happened in SPL
+		 * if (!tpm->tests_done)
+		 *    return TPM2_RC_NEEDS_TEST;
+		 */
 
 		break;
 	}
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 5/7] tpm: Allow reporting the internal state

[Simon Glass]

It is useful to read information about the current TPM state, where
supported, e.g. for debugging purposes when verified boot fails.

Add support for this to the TPM interface as well as Cr50. Add a simple
sandbox test.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 cmd/tpm-common.c               | 20 ++++++++++++++++++++
 cmd/tpm-user-utils.h           |  2 ++
 cmd/tpm-v2.c                   |  3 +++
 drivers/tpm/tpm-uclass.c       | 10 ++++++++++
 drivers/tpm/tpm2_tis_sandbox.c | 11 +++++++++++
 include/tpm-common.h           | 20 ++++++++++++++++++++
 test/dm/Makefile               |  1 +
 test/dm/tpm.c                  | 34 ++++++++++++++++++++++++++++++++++
 8 files changed, 101 insertions(+)
 create mode 100644 test/dm/tpm.c

diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c
index 47adaffd184..d0c63cadf41 100644
--- a/cmd/tpm-common.c
+++ b/cmd/tpm-common.c
@@ -333,6 +333,26 @@ int do_tpm_info(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	return 0;
 }
 
+int do_tpm_report_state(struct cmd_tbl *cmdtp, int flag, int argc,
+			char *const argv[])
+{
+	struct udevice *dev;
+	char buf[80];
+	int rc;
+
+	rc = get_tpm(&dev);
+	if (rc)
+		return rc;
+	rc = tpm_report_state(dev, buf, sizeof(buf));
+	if (rc < 0) {
+		printf("Couldn't get TPM state (%d)\n", rc);
+		return CMD_RET_FAILURE;
+	}
+	printf("%s\n", buf);
+
+	return 0;
+}
+
 int do_tpm_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct udevice *dev;
diff --git a/cmd/tpm-user-utils.h b/cmd/tpm-user-utils.h
index 358ddff5761..de4a934aab6 100644
--- a/cmd/tpm-user-utils.h
+++ b/cmd/tpm-user-utils.h
@@ -21,6 +21,8 @@ int do_tpm_device(struct cmd_tbl *cmdtp, int flag, int argc,
 		  char *const argv[]);
 int do_tpm_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_tpm_info(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
+int do_tpm_report_state(struct cmd_tbl *cmdtp, int flag, int argc,
+			char *const argv[]);
 int do_tpm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 
 #endif /* __TPM_USER_UTILS_H */
diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
index 4ea5f9f094f..d93b83ada93 100644
--- a/cmd/tpm-v2.c
+++ b/cmd/tpm-v2.c
@@ -359,6 +359,7 @@ static int do_tpm_pcr_setauthvalue(struct cmd_tbl *cmdtp, int flag,
 static struct cmd_tbl tpm2_commands[] = {
 	U_BOOT_CMD_MKENT(device, 0, 1, do_tpm_device, "", ""),
 	U_BOOT_CMD_MKENT(info, 0, 1, do_tpm_info, "", ""),
+	U_BOOT_CMD_MKENT(state, 0, 1, do_tpm_report_state, "", ""),
 	U_BOOT_CMD_MKENT(init, 0, 1, do_tpm_init, "", ""),
 	U_BOOT_CMD_MKENT(startup, 0, 1, do_tpm2_startup, "", ""),
 	U_BOOT_CMD_MKENT(self_test, 0, 1, do_tpm2_self_test, "", ""),
@@ -389,6 +390,8 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
 "    Show all devices or set the specified device\n"
 "info\n"
 "    Show information about the TPM.\n"
+"state\n"
+"    Show internal state from the TPM (if available)\n"
 "init\n"
 "    Initialize the software stack. Always the first command to issue.\n"
 "startup <mode>\n"
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index 0eb35f50c4e..5ff0cd3958c 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -49,6 +49,16 @@ int tpm_get_desc(struct udevice *dev, char *buf, int size)
 	return ops->get_desc(dev, buf, size);
 }
 
+int tpm_report_state(struct udevice *dev, char *buf, int size)
+{
+	struct tpm_ops *ops = tpm_get_ops(dev);
+
+	if (!ops->report_state)
+		return -ENOSYS;
+
+	return ops->report_state(dev, buf, size);
+}
+
 /* Returns max number of milliseconds to wait */
 static ulong tpm_tis_i2c_calc_ordinal_duration(struct tpm_chip_priv *priv,
 					       u32 ordinal)
diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c
index c26f5d35abf..dd94bdc31fb 100644
--- a/drivers/tpm/tpm2_tis_sandbox.c
+++ b/drivers/tpm/tpm2_tis_sandbox.c
@@ -795,6 +795,16 @@ static int sandbox_tpm2_get_desc(struct udevice *dev, char *buf, int size)
 	return snprintf(buf, size, "Sandbox TPM2.x");
 }
 
+static int sandbox_tpm2_report_state(struct udevice *dev, char *buf, int size)
+{
+	struct sandbox_tpm2 *priv = dev_get_priv(dev);
+
+	if (size < 40)
+		return -ENOSPC;
+
+	return snprintf(buf, size, "init_done=%d", priv->init_done);
+}
+
 static int sandbox_tpm2_open(struct udevice *dev)
 {
 	struct sandbox_tpm2 *tpm = dev_get_priv(dev);
@@ -834,6 +844,7 @@ static const struct tpm_ops sandbox_tpm2_ops = {
 	.open		= sandbox_tpm2_open,
 	.close		= sandbox_tpm2_close,
 	.get_desc	= sandbox_tpm2_get_desc,
+	.report_state	= sandbox_tpm2_report_state,
 	.xfer		= sandbox_tpm2_xfer,
 };
 
diff --git a/include/tpm-common.h b/include/tpm-common.h
index a28629e7013..b2c5404430f 100644
--- a/include/tpm-common.h
+++ b/include/tpm-common.h
@@ -119,6 +119,16 @@ struct tpm_ops {
 	 */
 	int (*get_desc)(struct udevice *dev, char *buf, int size);
 
+	/**
+	 * report_state() - Collect information about the current TPM state
+	 *
+	 * @dev:	Device to check
+	 * @buf:	Buffer to put the string
+	 * @size:	Maximum size of buffer
+	 * Return: return code of the operation (0 = success)
+	 */
+	int (*report_state)(struct udevice *dev, char *buf, int size);
+
 	/**
 	 * send() - send data to the TPM
 	 *
@@ -234,6 +244,16 @@ u32 tpm_clear_and_reenable(struct udevice *dev);
  */
 int tpm_get_desc(struct udevice *dev, char *buf, int size);
 
+/**
+ * tpm_report_state() - Collect information about the current TPM state
+ *
+ * @dev:	Device to check
+ * @buf:	Buffer to put the string
+ * @size:	Maximum size of buffer
+ * Return: return code of the operation (0 = success)
+ */
+int tpm_report_state(struct udevice *dev, char *buf, int size);
+
 /**
  * tpm_xfer() - send data to the TPM and get response
  *
diff --git a/test/dm/Makefile b/test/dm/Makefile
index 52fe178a828..7543df8823c 100644
--- a/test/dm/Makefile
+++ b/test/dm/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_SYSINFO_GPIO) += sysinfo-gpio.o
 obj-$(CONFIG_UT_DM) += tag.o
 obj-$(CONFIG_TEE) += tee.o
 obj-$(CONFIG_TIMER) += timer.o
+obj-$(CONFIG_TPM_V2) += tpm.o
 obj-$(CONFIG_DM_USB) += usb.o
 obj-$(CONFIG_DM_VIDEO) += video.o
 ifeq ($(CONFIG_VIRTIO_SANDBOX),y)
diff --git a/test/dm/tpm.c b/test/dm/tpm.c
new file mode 100644
index 00000000000..0b46f799591
--- /dev/null
+++ b/test/dm/tpm.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2022 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <tpm_api.h>
+#include <dm/test.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+/* Basic test of the TPM uclass */
+static int dm_test_tpm(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	char buf[50];
+
+	/* check probe success */
+	ut_assertok(uclass_first_device_err(UCLASS_TPM, &dev));
+	ut_assert(tpm_is_v2(dev));
+
+	ut_assert(tpm_report_state(dev, buf, sizeof(buf)));
+	ut_asserteq_str("init_done=0", buf);
+
+	ut_assertok(tpm_init(dev));
+
+	ut_assert(tpm_report_state(dev, buf, sizeof(buf)));
+	ut_asserteq_str("init_done=1", buf);
+
+	return 0;
+}
+DM_TEST(dm_test_tpm, UT_TESTF_SCAN_FDT);
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 6/7] tpm: Implement state command for Cr50

[Simon Glass]

Add a vendor-specific TPM2 command for this and implement it for Cr50.
Note: This is not part of the TPM spec, but is a Cr50 extension.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
 include/tpm-v2.h       |  54 +++++++++++++++++++
 lib/tpm-v2.c           |  24 +++++++++
 3 files changed, 195 insertions(+)

diff --git a/drivers/tpm/cr50_i2c.c b/drivers/tpm/cr50_i2c.c
index f8c30878947..dabf617be0e 100644
--- a/drivers/tpm/cr50_i2c.c
+++ b/drivers/tpm/cr50_i2c.c
@@ -13,11 +13,13 @@
 #include <irq.h>
 #include <log.h>
 #include <spl.h>
+#include <tpm-common.h>
 #include <tpm-v2.h>
 #include <acpi/acpigen.h>
 #include <acpi/acpi_device.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
+#include <asm/unaligned.h>
 #include <linux/delay.h>
 #include <dm/acpi.h>
 
@@ -54,6 +56,41 @@ struct cr50_priv {
 	bool use_irq;
 };
 
+/*
+ * The below structure represents the body of the response to the 'report tpm
+ * state' vendor command.
+ *
+ * It is transferred over the wire, so it needs to be serialized/deserialized,
+ * and it is likely to change, so its contents must be versioned.
+ */
+#define TPM_STATE_VERSION	1
+struct tpm_vendor_state {
+	u32 version;
+	/*
+	 * The following three fields are set by the TPM in case of an assert.
+	 * There is no other processing than setting the source code line
+	 * number, error code and the first 4 characters of the function name.
+	 *
+	 * We don't expect this happening, but it is included in the report
+	 * just in case.
+	 */
+	u32 fail_line;	/* s_failLIne */
+	u32 fail_code;	/* s_failCode */
+	char func_name[4];	/* s_failFunction, limited to 4 chars */
+
+	/*
+	 * The following two fields are the current time filtered value of the
+	 * 'failed tries' TPM counter, and the maximum allowed value of the
+	 * counter.
+	 *
+	 * failed_tries == max_tries is the definition of the TPM lockout
+	 * condition.
+	 */
+	u32 failed_tries;	/* gp.failedTries */
+	u32 max_tries;	/* gp.maxTries */
+	/* The below fields are present in version 2 and above */
+};
+
 /* Wait for interrupt to indicate TPM is ready */
 static int cr50_i2c_wait_tpm_ready(struct udevice *dev)
 {
@@ -573,6 +610,85 @@ static int cr50_i2c_get_desc(struct udevice *dev, char *buf, int size)
 	return len;
 }
 
+static int stringify_state(char *buf, int len, char *str, size_t max_size)
+{
+	struct tpm_vendor_state state;
+	size_t text_size = 0;
+
+	state.version = get_unaligned_be32(buf +
+		offsetof(struct tpm_vendor_state, version));
+	state.fail_line = get_unaligned_be32(buf +
+		offsetof(struct tpm_vendor_state, fail_line));
+	state.fail_code = get_unaligned_be32(buf +
+		offsetof(struct tpm_vendor_state, fail_code));
+	memcpy(state.func_name,
+	       buf + offsetof(struct tpm_vendor_state, func_name),
+	       sizeof(state.func_name));
+	state.failed_tries = get_unaligned_be32(buf +
+		offsetof(struct tpm_vendor_state, failed_tries));
+	state.max_tries = get_unaligned_be32(buf +
+		offsetof(struct tpm_vendor_state, max_tries));
+
+	text_size += snprintf(str + text_size, max_size - text_size,
+			      "v=%d", state.version);
+	if (text_size >= max_size)
+		return -ENOSPC;
+
+	if (state.version > TPM_STATE_VERSION)
+		text_size += snprintf(str + text_size,
+				      max_size - text_size,
+				      " not fully supported\n");
+	if (text_size >= max_size)
+		return -ENOSPC;
+
+	if (state.version == 0)
+		return -EINVAL;	/* This should never happen */
+
+	text_size += snprintf(str + text_size,
+			      max_size - text_size,
+			      " failed_tries=%d max_tries=%d\n",
+			      state.failed_tries, state.max_tries);
+	if (text_size >= max_size)
+		return -ENOSPC;
+
+	if (state.fail_line) {
+		/* make sure function name is zero terminated. */
+		char func_name[sizeof(state.func_name) + 1];
+
+		memcpy(func_name, state.func_name, sizeof(state.func_name));
+		func_name[sizeof(state.func_name)] = '\0';
+
+		text_size += snprintf(str + text_size,
+				      max_size - text_size,
+				      "tpm failed: f %s line %d code %d",
+				      func_name,
+				      state.fail_line,
+				      state.fail_code);
+		if (text_size >= max_size)
+			return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static int cr50_i2c_report_state(struct udevice *dev, char *str, int str_max)
+{
+	char buf[50];
+	int buf_size = sizeof(buf);
+	int ret;
+
+	ret = tpm2_cr50_report_state(dev, buf, &buf_size);
+	if (ret)
+		return ret;
+
+	/* TPM responded as expected */
+	ret = stringify_state(buf, buf_size, str, str_max);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int cr50_i2c_open(struct udevice *dev)
 {
 	char buf[80];
@@ -730,6 +846,7 @@ struct acpi_ops cr50_acpi_ops = {
 static const struct tpm_ops cr50_i2c_ops = {
 	.open		= cr50_i2c_open,
 	.get_desc	= cr50_i2c_get_desc,
+	.report_state	= cr50_i2c_report_state,
 	.send		= cr50_i2c_send,
 	.recv		= cr50_i2c_recv,
 	.cleanup	= cr50_i2c_cleanup,
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index e79c90b9395..8e90a616220 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -419,6 +419,50 @@ enum {
 	HR_NV_INDEX		= TPM_HT_NV_INDEX << HR_SHIFT,
 };
 
+/*
+ * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
+ *
+ * FIXME: below is not enough to differentiate between vendors commands
+ * of numerous devices. However, the current tpm2 APIs aren't very amenable
+ * to extending generically because the marshaling code is assuming all
+ * knowledge of all commands.
+ */
+#define TPM2_CC_VENDOR_BIT_MASK			0x20000000
+
+#define TPM2_CR50_VENDOR_COMMAND		(TPM2_CC_VENDOR_BIT_MASK | 0)
+#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET	19
+#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS	21
+#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE	23
+#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON	24
+#define TPM2_CR50_SUB_CMD_GET_REC_BTN		29
+#define TPM2_CR50_SUB_CMD_TPM_MODE		40
+#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE		52
+#define TPM2_CR50_SUB_CMD_RESET_EC		53
+
+/* Cr50 vendor-specific error codes. */
+#define VENDOR_RC_ERR              0x00000500
+enum cr50_vendor_rc {
+	VENDOR_RC_INTERNAL_ERROR	= (VENDOR_RC_ERR | 6),
+	VENDOR_RC_NO_SUCH_SUBCOMMAND	= (VENDOR_RC_ERR | 8),
+	VENDOR_RC_NO_SUCH_COMMAND	= (VENDOR_RC_ERR | 127),
+};
+
+enum cr50_tpm_mode {
+	/*
+	 * Default state: TPM is enabled, and may be set to either
+	 * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
+	 */
+	TPM_MODE_ENABLED_TENTATIVE = 0,
+
+	/* TPM is enabled, and mode may not be changed. */
+	TPM_MODE_ENABLED = 1,
+
+	/* TPM is disabled, and mode may not be changed. */
+	TPM_MODE_DISABLED = 2,
+
+	TPM_MODE_INVALID,
+};
+
 /**
  * Issue a TPM2_Startup command.
  *
@@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
 u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
 			u8 *recvbuf, size_t *recv_size);
 
+/**
+ * tpm_cr50_report_state() - Report the Cr50 internal state
+ *
+ * @dev:	TPM device
+ * @recvbuf:	Buffer to save the response to
+ * @recv_size:	Pointer to the size of the response buffer
+ * Return: result of the operation
+ */
+u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
+
 #endif /* __TPM_V2_H */
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 3e240bb4c67..3de4841974a 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
 {
 	return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
 }
+
+u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
+{
+	u8 command_v2[COMMAND_BUFFER_SIZE] = {
+		/* header 10 bytes */
+		tpm_u16(TPM2_ST_NO_SESSIONS),		/* TAG */
+		tpm_u32(10 + 2),			/* Length */
+		tpm_u32(TPM2_CR50_VENDOR_COMMAND),	/* Command code */
+
+		tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
+	};
+	int ret;
+
+	ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
+	log_debug("ret=%s, %x\n", dev->name, ret);
+	if (ret)
+		return ret;
+	if (*recv_size < 12)
+		return -ENODATA;
+	*recv_size -= 12;
+	memcpy(recvbuf, recvbuf + 12, *recv_size);
+
+	return 0;
+}
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Ilias Apalodimas]

Hi Simon,

I know little of this device and the whole patch seems fine apart from
the definitions and declarations of the state functions.


On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
>

>
>  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
>  include/tpm-v2.h       |  54 +++++++++++++++++++
>  lib/tpm-v2.c           |  24 +++++++++

[...]

> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index e79c90b9395..8e90a616220 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -419,6 +419,50 @@ enum {
>         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
>  };
>
> +/*
> + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> + *
> + * FIXME: below is not enough to differentiate between vendors commands
> + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> + * to extending generically because the marshaling code is assuming all
> + * knowledge of all commands.
> + */
> +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> +
> +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> +
> +/* Cr50 vendor-specific error codes. */
> +#define VENDOR_RC_ERR              0x00000500
> +enum cr50_vendor_rc {
> +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> +};
> +
> +enum cr50_tpm_mode {
> +       /*
> +        * Default state: TPM is enabled, and may be set to either
> +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> +        */
> +       TPM_MODE_ENABLED_TENTATIVE = 0,
> +
> +       /* TPM is enabled, and mode may not be changed. */
> +       TPM_MODE_ENABLED = 1,
> +
> +       /* TPM is disabled, and mode may not be changed. */
> +       TPM_MODE_DISABLED = 2,
> +
> +       TPM_MODE_INVALID,
> +};
> +
>  /**
>   * Issue a TPM2_Startup command.
>   *
> @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
>  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
>                         u8 *recvbuf, size_t *recv_size);
>
> +/**
> + * tpm_cr50_report_state() - Report the Cr50 internal state
> + *
> + * @dev:       TPM device
> + * @recvbuf:   Buffer to save the response to
> + * @recv_size: Pointer to the size of the response buffer
> + * Return: result of the operation
> + */
> +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> +

I think we should keep the generic include files clean for hardware
specific details.

>  #endif /* __TPM_V2_H */
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 3e240bb4c67..3de4841974a 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
>  {
>         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
>  }
> +
> +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> +{
> +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> +               /* header 10 bytes */
> +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> +               tpm_u32(10 + 2),                        /* Length */
> +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> +
> +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> +       };
> +       int ret;
> +
> +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> +       log_debug("ret=%s, %x\n", dev->name, ret);
> +       if (ret)
> +               return ret;
> +       if (*recv_size < 12)
> +               return -ENODATA;
> +       *recv_size -= 12;
> +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> +
> +       return 0;
> +}

Same here, this functions seems ok but shouldn't land in the generic TPM API

Thanks
/Ilias
> --
> 2.37.1.595.g718a3a8f04-goog
>

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Simon Glass]

Hi Ilias,

On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> I know little of this device and the whole patch seems fine apart from
> the definitions and declarations of the state functions.
>
>
> On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> >
>
> >
> >  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> >  include/tpm-v2.h       |  54 +++++++++++++++++++
> >  lib/tpm-v2.c           |  24 +++++++++
>
> [...]
>
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index e79c90b9395..8e90a616220 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -419,6 +419,50 @@ enum {
> >         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
> >  };
> >
> > +/*
> > + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> > + *
> > + * FIXME: below is not enough to differentiate between vendors commands
> > + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> > + * to extending generically because the marshaling code is assuming all
> > + * knowledge of all commands.
> > + */
> > +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> > +
> > +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> > +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> > +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> > +
> > +/* Cr50 vendor-specific error codes. */
> > +#define VENDOR_RC_ERR              0x00000500
> > +enum cr50_vendor_rc {
> > +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> > +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> > +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> > +};
> > +
> > +enum cr50_tpm_mode {
> > +       /*
> > +        * Default state: TPM is enabled, and may be set to either
> > +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > +        */
> > +       TPM_MODE_ENABLED_TENTATIVE = 0,
> > +
> > +       /* TPM is enabled, and mode may not be changed. */
> > +       TPM_MODE_ENABLED = 1,
> > +
> > +       /* TPM is disabled, and mode may not be changed. */
> > +       TPM_MODE_DISABLED = 2,
> > +
> > +       TPM_MODE_INVALID,
> > +};
> > +
> >  /**
> >   * Issue a TPM2_Startup command.
> >   *
> > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
> >  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> >                         u8 *recvbuf, size_t *recv_size);
> >
> > +/**
> > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > + *
> > + * @dev:       TPM device
> > + * @recvbuf:   Buffer to save the response to
> > + * @recv_size: Pointer to the size of the response buffer
> > + * Return: result of the operation
> > + */
> > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> > +
>
> I think we should keep the generic include files clean for hardware
> specific details.
>
> >  #endif /* __TPM_V2_H */
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 3e240bb4c67..3de4841974a 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> >  {
> >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> >  }
> > +
> > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> > +{
> > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > +               /* header 10 bytes */
> > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > +               tpm_u32(10 + 2),                        /* Length */
> > +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> > +
> > +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > +       };
> > +       int ret;
> > +
> > +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > +       log_debug("ret=%s, %x\n", dev->name, ret);
> > +       if (ret)
> > +               return ret;
> > +       if (*recv_size < 12)
> > +               return -ENODATA;
> > +       *recv_size -= 12;
> > +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> > +
> > +       return 0;
> > +}
>
> Same here, this functions seems ok but shouldn't land in the generic TPM API

So shall I create a new tpm_cr50.h header file? What about the C file?

>
> Thanks
> /Ilias
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >

Regards,
Simon

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Ilias Apalodimas]

Hi Simon,

On Wed, 17 Aug 2022 at 21:54, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > I know little of this device and the whole patch seems fine apart from
> > the definitions and declarations of the state functions.
> >
> >
> > On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> > >
> >
> > >
> > >  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> > >  include/tpm-v2.h       |  54 +++++++++++++++++++
> > >  lib/tpm-v2.c           |  24 +++++++++
> >
> > [...]
> >
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index e79c90b9395..8e90a616220 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -419,6 +419,50 @@ enum {
> > >         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
> > >  };
> > >
> > > +/*
> > > + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> > > + *
> > > + * FIXME: below is not enough to differentiate between vendors commands
> > > + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> > > + * to extending generically because the marshaling code is assuming all
> > > + * knowledge of all commands.
> > > + */
> > > +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> > > +
> > > +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> > > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> > > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> > > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> > > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> > > +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> > > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> > > +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> > > +
> > > +/* Cr50 vendor-specific error codes. */
> > > +#define VENDOR_RC_ERR              0x00000500
> > > +enum cr50_vendor_rc {
> > > +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> > > +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> > > +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> > > +};
> > > +
> > > +enum cr50_tpm_mode {
> > > +       /*
> > > +        * Default state: TPM is enabled, and may be set to either
> > > +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > > +        */
> > > +       TPM_MODE_ENABLED_TENTATIVE = 0,
> > > +
> > > +       /* TPM is enabled, and mode may not be changed. */
> > > +       TPM_MODE_ENABLED = 1,
> > > +
> > > +       /* TPM is disabled, and mode may not be changed. */
> > > +       TPM_MODE_DISABLED = 2,
> > > +
> > > +       TPM_MODE_INVALID,
> > > +};
> > > +
> > >  /**
> > >   * Issue a TPM2_Startup command.
> > >   *
> > > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
> > >  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > >                         u8 *recvbuf, size_t *recv_size);
> > >
> > > +/**
> > > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > > + *
> > > + * @dev:       TPM device
> > > + * @recvbuf:   Buffer to save the response to
> > > + * @recv_size: Pointer to the size of the response buffer
> > > + * Return: result of the operation
> > > + */
> > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> > > +
> >
> > I think we should keep the generic include files clean for hardware
> > specific details.
> >
> > >  #endif /* __TPM_V2_H */
> > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > index 3e240bb4c67..3de4841974a 100644
> > > --- a/lib/tpm-v2.c
> > > +++ b/lib/tpm-v2.c
> > > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > >  {
> > >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > >  }
> > > +
> > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> > > +{
> > > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > > +               /* header 10 bytes */
> > > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > > +               tpm_u32(10 + 2),                        /* Length */
> > > +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> > > +
> > > +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > > +       };
> > > +       int ret;
> > > +
> > > +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > > +       log_debug("ret=%s, %x\n", dev->name, ret);
> > > +       if (ret)
> > > +               return ret;
> > > +       if (*recv_size < 12)
> > > +               return -ENODATA;
> > > +       *recv_size -= 12;
> > > +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> > > +
> > > +       return 0;
> > > +}
> >
> > Same here, this functions seems ok but shouldn't land in the generic TPM API
>
> So shall I create a new tpm_cr50.h header file? What about the C file?

Yea the header file seems fine (I assume in drivers/tpm?)

About the C file, tpm2_cr50_report_state() is called by
cr50_i2c_report_state() which is static in the drivers.  Can't we just
move the function there?

Regards
/Ilias
>
> >
> > Thanks
> > /Ilias
> > > --
> > > 2.37.1.595.g718a3a8f04-goog
> > >
>
> Regards,
> Simon

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Simon Glass]

Hi Ilias,

On Thu, 18 Aug 2022 at 01:29, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 17 Aug 2022 at 21:54, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > I know little of this device and the whole patch seems fine apart from
> > > the definitions and declarations of the state functions.
> > >
> > >
> > > On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > >
> > > >
> > > >  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> > > >  include/tpm-v2.h       |  54 +++++++++++++++++++
> > > >  lib/tpm-v2.c           |  24 +++++++++
> > >
> > > [...]
> > >
> > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > index e79c90b9395..8e90a616220 100644
> > > > --- a/include/tpm-v2.h
> > > > +++ b/include/tpm-v2.h
> > > > @@ -419,6 +419,50 @@ enum {
> > > >         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
> > > >  };
> > > >
> > > > +/*
> > > > + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> > > > + *
> > > > + * FIXME: below is not enough to differentiate between vendors commands
> > > > + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> > > > + * to extending generically because the marshaling code is assuming all
> > > > + * knowledge of all commands.
> > > > + */
> > > > +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> > > > +
> > > > +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> > > > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> > > > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > > > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> > > > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> > > > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> > > > +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> > > > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> > > > +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> > > > +
> > > > +/* Cr50 vendor-specific error codes. */
> > > > +#define VENDOR_RC_ERR              0x00000500
> > > > +enum cr50_vendor_rc {
> > > > +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> > > > +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> > > > +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> > > > +};
> > > > +
> > > > +enum cr50_tpm_mode {
> > > > +       /*
> > > > +        * Default state: TPM is enabled, and may be set to either
> > > > +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > > > +        */
> > > > +       TPM_MODE_ENABLED_TENTATIVE = 0,
> > > > +
> > > > +       /* TPM is enabled, and mode may not be changed. */
> > > > +       TPM_MODE_ENABLED = 1,
> > > > +
> > > > +       /* TPM is disabled, and mode may not be changed. */
> > > > +       TPM_MODE_DISABLED = 2,
> > > > +
> > > > +       TPM_MODE_INVALID,
> > > > +};
> > > > +
> > > >  /**
> > > >   * Issue a TPM2_Startup command.
> > > >   *
> > > > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
> > > >  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > >                         u8 *recvbuf, size_t *recv_size);
> > > >
> > > > +/**
> > > > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > > > + *
> > > > + * @dev:       TPM device
> > > > + * @recvbuf:   Buffer to save the response to
> > > > + * @recv_size: Pointer to the size of the response buffer
> > > > + * Return: result of the operation
> > > > + */
> > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> > > > +
> > >
> > > I think we should keep the generic include files clean for hardware
> > > specific details.
> > >
> > > >  #endif /* __TPM_V2_H */
> > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > > index 3e240bb4c67..3de4841974a 100644
> > > > --- a/lib/tpm-v2.c
> > > > +++ b/lib/tpm-v2.c
> > > > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > >  {
> > > >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > > >  }
> > > > +
> > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> > > > +{
> > > > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > > > +               /* header 10 bytes */
> > > > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > > > +               tpm_u32(10 + 2),                        /* Length */
> > > > +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> > > > +
> > > > +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > > > +       };
> > > > +       int ret;
> > > > +
> > > > +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > > > +       log_debug("ret=%s, %x\n", dev->name, ret);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +       if (*recv_size < 12)
> > > > +               return -ENODATA;
> > > > +       *recv_size -= 12;
> > > > +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> > > > +
> > > > +       return 0;
> > > > +}
> > >
> > > Same here, this functions seems ok but shouldn't land in the generic TPM API
> >
> > So shall I create a new tpm_cr50.h header file? What about the C file?
>
> Yea the header file seems fine (I assume in drivers/tpm?)
>
> About the C file, tpm2_cr50_report_state() is called by
> cr50_i2c_report_state() which is static in the drivers.  Can't we just
> move the function there?

Just digging into this but I think you have the wrong end of the stick.

I have added a new TPM method to the uclass for this. We cannot call
functions 'sideways' of that mechanism as it violates how driver model
works.

The feature is implemented for cr50 and sandbox. It could be
implemented for other TPMs if they have any info that can usefully be
provided, such as the state of the PCRs or something else useful or
important.

So I think my patches are already doing all this correctly. Can you
please take another look? I'll send v3 as Heinrich had a minor comment
on a function comment. Also one of your changes.

Regards,
Simon

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Ilias Apalodimas]

Hi Simon,

On Fri, 19 Aug 2022 at 16:47, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 18 Aug 2022 at 01:29, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Wed, 17 Aug 2022 at 21:54, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > I know little of this device and the whole patch seems fine apart from
> > > > the definitions and declarations of the state functions.
> > > >
> > > >
> > > > On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > >
> > > > >
> > > > >  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> > > > >  include/tpm-v2.h       |  54 +++++++++++++++++++
> > > > >  lib/tpm-v2.c           |  24 +++++++++
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > > index e79c90b9395..8e90a616220 100644
> > > > > --- a/include/tpm-v2.h
> > > > > +++ b/include/tpm-v2.h
> > > > > @@ -419,6 +419,50 @@ enum {
> > > > >         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
> > > > >  };
> > > > >
> > > > > +/*
> > > > > + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> > > > > + *
> > > > > + * FIXME: below is not enough to differentiate between vendors commands
> > > > > + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> > > > > + * to extending generically because the marshaling code is assuming all
> > > > > + * knowledge of all commands.
> > > > > + */
> > > > > +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> > > > > +
> > > > > +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> > > > > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> > > > > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > > > > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> > > > > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> > > > > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> > > > > +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> > > > > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> > > > > +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> > > > > +
> > > > > +/* Cr50 vendor-specific error codes. */
> > > > > +#define VENDOR_RC_ERR              0x00000500
> > > > > +enum cr50_vendor_rc {
> > > > > +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> > > > > +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> > > > > +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> > > > > +};
> > > > > +
> > > > > +enum cr50_tpm_mode {
> > > > > +       /*
> > > > > +        * Default state: TPM is enabled, and may be set to either
> > > > > +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > > > > +        */
> > > > > +       TPM_MODE_ENABLED_TENTATIVE = 0,
> > > > > +
> > > > > +       /* TPM is enabled, and mode may not be changed. */
> > > > > +       TPM_MODE_ENABLED = 1,
> > > > > +
> > > > > +       /* TPM is disabled, and mode may not be changed. */
> > > > > +       TPM_MODE_DISABLED = 2,
> > > > > +
> > > > > +       TPM_MODE_INVALID,
> > > > > +};
> > > > > +
> > > > >  /**
> > > > >   * Issue a TPM2_Startup command.
> > > > >   *
> > > > > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
> > > > >  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > > >                         u8 *recvbuf, size_t *recv_size);
> > > > >
> > > > > +/**
> > > > > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > > > > + *
> > > > > + * @dev:       TPM device
> > > > > + * @recvbuf:   Buffer to save the response to
> > > > > + * @recv_size: Pointer to the size of the response buffer
> > > > > + * Return: result of the operation
> > > > > + */
> > > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> > > > > +
> > > >
> > > > I think we should keep the generic include files clean for hardware
> > > > specific details.
> > > >
> > > > >  #endif /* __TPM_V2_H */
> > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > > > index 3e240bb4c67..3de4841974a 100644
> > > > > --- a/lib/tpm-v2.c
> > > > > +++ b/lib/tpm-v2.c
> > > > > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > > >  {
> > > > >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > > > >  }
> > > > > +
> > > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> > > > > +{
> > > > > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > > > > +               /* header 10 bytes */
> > > > > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > > > > +               tpm_u32(10 + 2),                        /* Length */
> > > > > +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> > > > > +
> > > > > +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > > > > +       };
> > > > > +       int ret;
> > > > > +
> > > > > +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > > > > +       log_debug("ret=%s, %x\n", dev->name, ret);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +       if (*recv_size < 12)
> > > > > +               return -ENODATA;
> > > > > +       *recv_size -= 12;
> > > > > +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > >
> > > > Same here, this functions seems ok but shouldn't land in the generic TPM API
> > >
> > > So shall I create a new tpm_cr50.h header file? What about the C file?
> >
> > Yea the header file seems fine (I assume in drivers/tpm?)
> >
> > About the C file, tpm2_cr50_report_state() is called by
> > cr50_i2c_report_state() which is static in the drivers.  Can't we just
> > move the function there?
>
> Just digging into this but I think you have the wrong end of the stick.
>
> I have added a new TPM method to the uclass for this. We cannot call
> functions 'sideways' of that mechanism as it violates how driver model
> works.
>

I am not sure I am following here.  I saw the uclass patches and they
seem fine.  The only thing that needs to be changed is move the cr50*
generic functions outside the lib/tpm-v2.c API. This seems doable and
the only thing that prevents us from doing it is some definitions in
lib/tpm-utils.h which the new functions need.

I'll go reply on the new patchset.

Thanks
/Ilias
> The feature is implemented for cr50 and sandbox. It could be
> implemented for other TPMs if they have any info that can usefully be
> provided, such as the state of the PCRs or something else useful or
> important.
>
> So I think my patches are already doing all this correctly. Can you
> please take another look? I'll send v3 as Heinrich had a minor comment
> on a function comment. Also one of your changes.
>
> Regards,
> Simon

Re: [PATCH v2 6/7] tpm: Implement state command for Cr50

[Simon Glass]

Hi Ilias,

On Mon, 22 Aug 2022 at 00:00, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 19 Aug 2022 at 16:47, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 18 Aug 2022 at 01:29, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, 17 Aug 2022 at 21:54, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > I know little of this device and the whole patch seems fine apart from
> > > > > the definitions and declarations of the state functions.
> > > > >
> > > > >
> > > > > On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > >
> > > > > >
> > > > > >  drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/tpm-v2.h       |  54 +++++++++++++++++++
> > > > > >  lib/tpm-v2.c           |  24 +++++++++
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > > > index e79c90b9395..8e90a616220 100644
> > > > > > --- a/include/tpm-v2.h
> > > > > > +++ b/include/tpm-v2.h
> > > > > > @@ -419,6 +419,50 @@ enum {
> > > > > >         HR_NV_INDEX             = TPM_HT_NV_INDEX << HR_SHIFT,
> > > > > >  };
> > > > > >
> > > > > > +/*
> > > > > > + * Operations specific to the Cr50 TPM used on Chromium OS and Android devices
> > > > > > + *
> > > > > > + * FIXME: below is not enough to differentiate between vendors commands
> > > > > > + * of numerous devices. However, the current tpm2 APIs aren't very amenable
> > > > > > + * to extending generically because the marshaling code is assuming all
> > > > > > + * knowledge of all commands.
> > > > > > + */
> > > > > > +#define TPM2_CC_VENDOR_BIT_MASK                        0x20000000
> > > > > > +
> > > > > > +#define TPM2_CR50_VENDOR_COMMAND               (TPM2_CC_VENDOR_BIT_MASK | 0)
> > > > > > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET      19
> > > > > > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > > > > > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE     23
> > > > > > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON       24
> > > > > > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN          29
> > > > > > +#define TPM2_CR50_SUB_CMD_TPM_MODE             40
> > > > > > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE                52
> > > > > > +#define TPM2_CR50_SUB_CMD_RESET_EC             53
> > > > > > +
> > > > > > +/* Cr50 vendor-specific error codes. */
> > > > > > +#define VENDOR_RC_ERR              0x00000500
> > > > > > +enum cr50_vendor_rc {
> > > > > > +       VENDOR_RC_INTERNAL_ERROR        = (VENDOR_RC_ERR | 6),
> > > > > > +       VENDOR_RC_NO_SUCH_SUBCOMMAND    = (VENDOR_RC_ERR | 8),
> > > > > > +       VENDOR_RC_NO_SUCH_COMMAND       = (VENDOR_RC_ERR | 127),
> > > > > > +};
> > > > > > +
> > > > > > +enum cr50_tpm_mode {
> > > > > > +       /*
> > > > > > +        * Default state: TPM is enabled, and may be set to either
> > > > > > +        * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > > > > > +        */
> > > > > > +       TPM_MODE_ENABLED_TENTATIVE = 0,
> > > > > > +
> > > > > > +       /* TPM is enabled, and mode may not be changed. */
> > > > > > +       TPM_MODE_ENABLED = 1,
> > > > > > +
> > > > > > +       /* TPM is disabled, and mode may not be changed. */
> > > > > > +       TPM_MODE_DISABLED = 2,
> > > > > > +
> > > > > > +       TPM_MODE_INVALID,
> > > > > > +};
> > > > > > +
> > > > > >  /**
> > > > > >   * Issue a TPM2_Startup command.
> > > > > >   *
> > > > > > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice *dev);
> > > > > >  u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > > > >                         u8 *recvbuf, size_t *recv_size);
> > > > > >
> > > > > > +/**
> > > > > > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > > > > > + *
> > > > > > + * @dev:       TPM device
> > > > > > + * @recvbuf:   Buffer to save the response to
> > > > > > + * @recv_size: Pointer to the size of the response buffer
> > > > > > + * Return: result of the operation
> > > > > > + */
> > > > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
> > > > > > +
> > > > >
> > > > > I think we should keep the generic include files clean for hardware
> > > > > specific details.
> > > > >
> > > > > >  #endif /* __TPM_V2_H */
> > > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > > > > index 3e240bb4c67..3de4841974a 100644
> > > > > > --- a/lib/tpm-v2.c
> > > > > > +++ b/lib/tpm-v2.c
> > > > > > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > > > >  {
> > > > > >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > > > > >  }
> > > > > > +
> > > > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
> > > > > > +{
> > > > > > +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > > > > > +               /* header 10 bytes */
> > > > > > +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> > > > > > +               tpm_u32(10 + 2),                        /* Length */
> > > > > > +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> > > > > > +
> > > > > > +               tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > > > > > +       };
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > > > > > +       log_debug("ret=%s, %x\n", dev->name, ret);
> > > > > > +       if (ret)
> > > > > > +               return ret;
> > > > > > +       if (*recv_size < 12)
> > > > > > +               return -ENODATA;
> > > > > > +       *recv_size -= 12;
> > > > > > +       memcpy(recvbuf, recvbuf + 12, *recv_size);
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > >
> > > > > Same here, this functions seems ok but shouldn't land in the generic TPM API
> > > >
> > > > So shall I create a new tpm_cr50.h header file? What about the C file?
> > >
> > > Yea the header file seems fine (I assume in drivers/tpm?)
> > >
> > > About the C file, tpm2_cr50_report_state() is called by
> > > cr50_i2c_report_state() which is static in the drivers.  Can't we just
> > > move the function there?
> >
> > Just digging into this but I think you have the wrong end of the stick.
> >
> > I have added a new TPM method to the uclass for this. We cannot call
> > functions 'sideways' of that mechanism as it violates how driver model
> > works.
> >
>
> I am not sure I am following here.  I saw the uclass patches and they
> seem fine.  The only thing that needs to be changed is move the cr50*
> generic functions outside the lib/tpm-v2.c API. This seems doable and
> the only thing that prevents us from doing it is some definitions in
> lib/tpm-utils.h which the new functions need.
>
> I'll go reply on the new patchset.

It would be good to get past this...see my comments on the other patch.

Regards,
Simon


>
> Thanks
> /Ilias
> > The feature is implemented for cr50 and sandbox. It could be
> > implemented for other TPMs if they have any info that can usefully be
> > provided, such as the state of the PCRs or something else useful or
> > important.
> >
> > So I think my patches are already doing all this correctly. Can you
> > please take another look? I'll send v3 as Heinrich had a minor comment
> > on a function comment. Also one of your changes.
> >
> > Regards,
> > Simon

[PATCH v2 7/7] tpm: Allow committing non-volatile data

[Simon Glass]

Add an option to tell the TPM to commit non-volatile data immediately it
is changed, rather than waiting until later. This is needed in some
situations, since if the device reboots it may not write the data.

Add definitions for the rest of the Cr50 commands while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 include/tpm-v2.h | 14 ++++++++++++++
 lib/tpm-v2.c     | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 8e90a616220..0a03994740d 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -712,4 +712,18 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
  */
 u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
 
+/*
+ * tpm2_cr50_enable_nvcommits() - Tell Cr50 to commit NV data immediately
+ *
+ * For Chromium OS verified boot, we may reboot or reset at different times,
+ * possibly leaving non-volatile data unwritten by the TPM.
+ *
+ * This vendor command is used to indicate that non-volatile data should be
+ * written to its store immediately.
+ *
+ * @dev		TPM device
+ * Return: result of the operation
+ */
+u32 tpm2_cr50_enable_nvcommits(struct udevice *dev);
+
 #endif /* __TPM_V2_H */
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 3de4841974a..d68c311651b 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -703,3 +703,23 @@ u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
 
 	return 0;
 }
+
+u32 tpm2_cr50_enable_nvcommits(struct udevice *dev)
+{
+	u8 command_v2[COMMAND_BUFFER_SIZE] = {
+		/* header 10 bytes */
+		tpm_u16(TPM2_ST_NO_SESSIONS),		/* TAG */
+		tpm_u32(10 + 2),			/* Length */
+		tpm_u32(TPM2_CR50_VENDOR_COMMAND),	/* Command code */
+
+		tpm_u16(TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS),
+	};
+	int ret;
+
+	ret = tpm_sendrecv_command(dev, command_v2, NULL, NULL);
+	log_debug("ret=%s, %x\n", dev->name, ret);
+	if (ret)
+		return ret;
+
+	return 0;
+}
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 7/7] tpm: Allow committing non-volatile data

[Ilias Apalodimas]

Hi Simon,

On Sat, 13 Aug 2022 at 22:56, Simon Glass <sjg@chromium.org> wrote:
>
> Add an option to tell the TPM to commit non-volatile data immediately it
> is changed, rather than waiting until later. This is needed in some
> situations, since if the device reboots it may not write the data.

Similar to the previous patch, I think this is fine, but the functions
don't belong to the generic TPM API

Regards
/Ilias
>
> Add definitions for the rest of the Cr50 commands while we are here.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  include/tpm-v2.h | 14 ++++++++++++++
>  lib/tpm-v2.c     | 20 ++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 8e90a616220..0a03994740d 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -712,4 +712,18 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
>   */
>  u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size);
>
> +/*
> + * tpm2_cr50_enable_nvcommits() - Tell Cr50 to commit NV data immediately
> + *
> + * For Chromium OS verified boot, we may reboot or reset at different times,
> + * possibly leaving non-volatile data unwritten by the TPM.
> + *
> + * This vendor command is used to indicate that non-volatile data should be
> + * written to its store immediately.
> + *
> + * @dev                TPM device
> + * Return: result of the operation
> + */
> +u32 tpm2_cr50_enable_nvcommits(struct udevice *dev);
> +
>  #endif /* __TPM_V2_H */
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 3de4841974a..d68c311651b 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -703,3 +703,23 @@ u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t *recv_size)
>
>         return 0;
>  }
> +
> +u32 tpm2_cr50_enable_nvcommits(struct udevice *dev)
> +{
> +       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> +               /* header 10 bytes */
> +               tpm_u16(TPM2_ST_NO_SESSIONS),           /* TAG */
> +               tpm_u32(10 + 2),                        /* Length */
> +               tpm_u32(TPM2_CR50_VENDOR_COMMAND),      /* Command code */
> +
> +               tpm_u16(TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS),
> +       };
> +       int ret;
> +
> +       ret = tpm_sendrecv_command(dev, command_v2, NULL, NULL);
> +       log_debug("ret=%s, %x\n", dev->name, ret);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> --
> 2.37.1.595.g718a3a8f04-goog
>