Hi Heinrich,

On Fri, 10 Jan 2025 at 19:06, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

Am 10. Januar 2025 22:56:34 MEZ schrieb Raymond Mao < raymond.mao@linaro.org>:

...

Move default eventlog size from efi to tpm for using in both modules.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v4

• Initial patch.

include/efi_tcg2.h | 2 -- include/tpm_tcg2.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h index 8dfb1bc952..7ed8880991 100644 --- a/include/efi_tcg2.h +++ b/include/efi_tcg2.h @@ -28,8 +28,6 @@ #define EFI_TCG2_MAX_PCR_INDEX 23 #define EFI_TCG2_FINAL_EVENTS_TABLE_VERSION 1

-#define TPM2_EVENT_LOG_SIZE CONFIG_EFI_TCG2_PROTOCOL_EVENTLOG_SIZE

typedef u32 efi_tcg_event_log_bitmap; typedef u32 efi_tcg_event_log_format; typedef u32 efi_tcg_event_algorithm_bitmap; diff --git a/include/tpm_tcg2.h b/include/tpm_tcg2.h index 6519004cc4..25a31daf65 100644 --- a/include/tpm_tcg2.h +++ b/include/tpm_tcg2.h @@ -65,6 +65,8 @@ #define EFI_DTB_EVENT_STRING \ "DTB DATA"

Please, provide Sphinx style documentation for defines.

...

+#define TPM2_EVENT_LOG_SIZE CONFIG_EFI_TCG2_PROTOCOL_EVENTLOG_SIZE

Why would we need such a define at all? Can't we directly use the CONFIG value?

Why should the CONFIG value remain in lib/efi_loader/Kconfig? I would not expect the log size in the non-EFI case to depend on the UEFI subsystem.

This eventlog size should be treated as part of the kconfigs of

TCG2_PROTOCOL but not lower level of TPM2. We don't have a separate kconfig page for TCG2_PROTOCOL, if my understanding is correct, all TCG2 stuff are supposed to be used by EFI only for now.

Regards, Raymond

...

/**

• struct TCG_EfiSpecIdEventAlgorithmSize - hashing algorithm

information

...

Am 10. Januar 2025 22:56:35 MEZ schrieb Raymond Mao raymond.mao@linaro.org:

Get tpm event log from bloblist instead of FDT when bloblist is enabled and valid from previous boot stage.

As a fallback, when no event log from previous stage is observed and no user buffer is passed, malloc a default buffer to initialize the event log.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Remove patch dependency.
• Remove the fallback to FDT when BLOBLIST is selected.

Changes in v3

• Malloc an 8KB buffer when user eventlog buffer does not exist.

Changes in v4

• Replace the default eventlog size with TPM2_EVENT_LOG_SIZE.

lib/tpm_tcg2.c | 55 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/lib/tpm_tcg2.c b/lib/tpm_tcg2.c index 7f868cc883..685699688b 100644 --- a/lib/tpm_tcg2.c +++ b/lib/tpm_tcg2.c @@ -5,6 +5,7 @@

#include <dm.h> #include <dm/of_access.h> +#include <malloc.h> #include <tpm_api.h> #include <tpm-common.h> #include <tpm-v2.h> @@ -19,6 +20,7 @@ #include <linux/unaligned/generic.h> #include <linux/unaligned/le_byteshift.h> #include "tpm-utils.h" +#include <bloblist.h>

int tcg2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr, u32 *pcr_banks) @@ -607,15 +609,24 @@ int tcg2_log_prepare_buffer(struct udevice *dev, struct tcg2_event_log *elog, elog->found = log.found; }

• if (elog->found)

• return 0;
  

• /*

• * Initialize the log buffer if no log was discovered and the buffer is
  

• * valid. User's can pass in their own buffer as a fallback if no
  

• * memory region is found.
  

• * Initialize the log buffer if no log was discovered.
  

• * User can pass in their own buffer as a fallback if no memory region
  

• * is found, else malloc a buffer if it does not exist.
  

  */

• if (!elog->found && elog->log_size)

• rc = tcg2_log_init(dev, elog);
  

• if (!elog->log_size) {

• elog->log = malloc(TPM2_EVENT_LOG_SIZE);
  

• if (!elog->log)
  

• 	return -ENOMEM;
  

• memset(elog->log, 0, TPM2_EVENT_LOG_SIZE);
  

• elog->log_size = TPM2_EVENT_LOG_SIZE;
  

• }

• return rc;

• return tcg2_log_init(dev, elog);

}

int tcg2_measurement_init(struct udevice **dev, struct tcg2_event_log *elog, @@ -668,10 +679,19 @@ __weak int tcg2_platform_get_log(struct udevice *dev, void **addr, u32 *size) const __be32 *size_prop; int asize; int ssize;

• struct ofnode_phandle_args args;

• phys_addr_t a;

• fdt_size_t s;

  *addr = NULL; *size = 0;

• *addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size);

• if (*addr && *size)

• return 0;
  

• else if (CONFIG_IS_ENABLED(BLOBLIST))

• return -ENODEV;
  

You are querying the CONFIG value. Why call function bloblist_get_blob if blobs are not supported? Please, simply skip in this case.

Best regards

Heinrich

addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize); if (!addr_prop) addr_prop = dev_read_prop(dev, "linux,sml-base", &asize); @@ -686,22 +706,19 @@ __weak int tcg2_platform_get_log(struct udevice *dev, void **addr, u32 *size)

*addr = map_physmem(a, s, MAP_NOCACHE);
*size = (u32)s;

• } else {

• struct ofnode_phandle_args args;
  

• phys_addr_t a;
  

• fdt_size_t s;
  
  

• if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0,
  

• 			       0, &args))
  

• 	return -ENODEV;
  

• return 0;
  

• }

• a = ofnode_get_addr_size(args.node, "reg", &s);
  

• if (a == FDT_ADDR_T_NONE)
  

• 	return -ENOMEM;
  

• if (dev_read_phandle_with_args(dev, "memory-region", NULL, 0, 0, &args))

• return -ENODEV;
  
  

• *addr = map_physmem(a, s, MAP_NOCACHE);
  

• *size = (u32)s;
  

• }

• a = ofnode_get_addr_size(args.node, "reg", &s);

• if (a == FDT_ADDR_T_NONE)

• return -ENOMEM;
  

• *addr = map_physmem(a, s, MAP_NOCACHE);

• *size = (u32)s;

  return 0;

}

On 13.01.25 15:43, Raymond Mao wrote:

Hi Heinrich,

On Fri, 10 Jan 2025 at 19:12, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

Am 10. Januar 2025 22:56:35 MEZ schrieb Raymond Mao
<raymond.mao@linaro.org <mailto:raymond.mao@linaro.org>>:
 >Get tpm event log from bloblist instead of FDT when bloblist is
 >enabled and valid from previous boot stage.
 >
 >As a fallback, when no event log from previous stage is observed
 >and no user buffer is passed, malloc a default buffer to initialize
 >the event log.
 >
 >Signed-off-by: Raymond Mao <raymond.mao@linaro.org
<mailto:raymond.mao@linaro.org>>
 >---
 >Changes in v2
 >- Remove patch dependency.
 >- Remove the fallback to FDT when BLOBLIST is selected.
 >Changes in v3
 >- Malloc an 8KB buffer when user eventlog buffer does not exist.
 >Changes in v4
 >- Replace the default eventlog size with TPM2_EVENT_LOG_SIZE.
 >
 > lib/tpm_tcg2.c | 55 ++++++++++++++++++++++++++++++++
+-----------------
 > 1 file changed, 36 insertions(+), 19 deletions(-)
 >
 >diff --git a/lib/tpm_tcg2.c b/lib/tpm_tcg2.c
 >index 7f868cc883..685699688b 100644
 >--- a/lib/tpm_tcg2.c
 >+++ b/lib/tpm_tcg2.c
 >@@ -5,6 +5,7 @@
 >
 > #include <dm.h>
 > #include <dm/of_access.h>
 >+#include <malloc.h>
 > #include <tpm_api.h>
 > #include <tpm-common.h>
 > #include <tpm-v2.h>
 >@@ -19,6 +20,7 @@
 > #include <linux/unaligned/generic.h>
 > #include <linux/unaligned/le_byteshift.h>
 > #include "tpm-utils.h"
 >+#include <bloblist.h>
 >
 > int tcg2_get_pcr_info(struct udevice *dev, u32 *supported_pcr,
u32 *active_pcr,
 >                     u32 *pcr_banks)
 >@@ -607,15 +609,24 @@ int tcg2_log_prepare_buffer(struct udevice
*dev, struct tcg2_event_log *elog,
 >               elog->found = log.found;
 >       }
 >
 >+      if (elog->found)
 >+              return 0;
 >+
 >       /*
 >-       * Initialize the log buffer if no log was discovered and
the buffer is
 >-       * valid. User's can pass in their own buffer as a fallback
if no
 >-       * memory region is found.
 >+       * Initialize the log buffer if no log was discovered.
 >+       * User can pass in their own buffer as a fallback if no
memory region
 >+       * is found, else malloc a buffer if it does not exist.
 >        */
 >-      if (!elog->found && elog->log_size)
 >-              rc = tcg2_log_init(dev, elog);
 >+      if (!elog->log_size) {
 >+              elog->log = malloc(TPM2_EVENT_LOG_SIZE);
 >+              if (!elog->log)
 >+                      return -ENOMEM;
 >+
 >+              memset(elog->log, 0, TPM2_EVENT_LOG_SIZE);
 >+              elog->log_size = TPM2_EVENT_LOG_SIZE;
 >+      }
 >
 >-      return rc;
 >+      return tcg2_log_init(dev, elog);
 > }
 >
 > int tcg2_measurement_init(struct udevice **dev, struct
tcg2_event_log *elog,
 >@@ -668,10 +679,19 @@ __weak int tcg2_platform_get_log(struct
udevice *dev, void **addr, u32 *size)
 >       const __be32 *size_prop;
 >       int asize;
 >       int ssize;
 >+      struct ofnode_phandle_args args;
 >+      phys_addr_t a;
 >+      fdt_size_t s;
 >
 >       *addr = NULL;
 >       *size = 0;
 >
 >+      *addr = bloblist_get_blob(BLOBLISTT_TPM_EVLOG, size);
 >+      if (*addr && *size)
 >+              return 0;
 >+      else if (CONFIG_IS_ENABLED(BLOBLIST))
 >+              return -ENODEV;
 >+

You are querying the CONFIG value. Why call function
bloblist_get_blob if blobs are not supported? Please, simply skip in
this case.

Actually BLOBLIST is not required to call bloblist_get_blob here since I have added the inline function, but we need a kconfig to separate the user's preference on "fallback to DT" or not. BLOBLIST is not a perfect idea but a temporary solution before we have a concrete idea to define one new kconfig for "blobs handoff from previous boot stage is mandatory". For more information, please see the previous discussion between me, Tom and Simon in v1 of this patch.

Why should we call the empty inline function, if we already check the CONFIG value?

Best regards

Heinrich

Regards, Raymond

 >       addr_prop = dev_read_prop(dev, "tpm_event_log_addr", &asize);
 >       if (!addr_prop)
 >               addr_prop = dev_read_prop(dev, "linux,sml-base",
&asize);
 >@@ -686,22 +706,19 @@ __weak int tcg2_platform_get_log(struct
udevice *dev, void **addr, u32 *size)
 >
 >               *addr = map_physmem(a, s, MAP_NOCACHE);
 >               *size = (u32)s;
 >-      } else {
 >-              struct ofnode_phandle_args args;
 >-              phys_addr_t a;
 >-              fdt_size_t s;
 >
 >-              if (dev_read_phandle_with_args(dev, "memory-
region", NULL, 0,
 >-                                             0, &args))
 >-                      return -ENODEV;
 >+              return 0;
 >+      }
 >
 >-              a = ofnode_get_addr_size(args.node, "reg", &s);
 >-              if (a == FDT_ADDR_T_NONE)
 >-                      return -ENOMEM;
 >+      if (dev_read_phandle_with_args(dev, "memory-region", NULL,
0, 0, &args))
 >+              return -ENODEV;
 >
 >-              *addr = map_physmem(a, s, MAP_NOCACHE);
 >-              *size = (u32)s;
 >-      }
 >+      a = ofnode_get_addr_size(args.node, "reg", &s);
 >+      if (a == FDT_ADDR_T_NONE)
 >+              return -ENOMEM;
 >+
 >+      *addr = map_physmem(a, s, MAP_NOCACHE);
 >+      *size = (u32)s;
 >
 >       return 0;
 > }

Hi Heinrich,

On Fri, 10 Jan 2025 at 19:04, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao < raymond.mao@linaro.org>:

...

bloblist_find function only returns the pointer of blob data, which is fine for those self-describing data like FDT. But as a common scenario, an interface is needed to retrieve both the pointer and the size of the blob data.

Add a few ut test cases for the new api.

Signed-off-by: Raymond Mao raymond.mao@linaro.org Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Changes in v2

• Rename function argument.
• Add ut tests.

Changes in v3

• None.

Changes in v4

• None.

common/bloblist.c | 17 +++++++++++++++-- include/bloblist.h | 18 ++++++++++++++++++ test/common/bloblist.c | 4 ++++ 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/common/bloblist.c b/common/bloblist.c index 110bb9dc44..ab48a3cdb9 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag, struct

bloblist_rec **recp, int size,

...

}

void *bloblist_find(uint tag, int size) +{

•  void *blob = NULL;
  

•  int blob_size;
  

•  blob = bloblist_get_blob(tag, &blob_size);
  

•  if (size && size != blob_size)
  

•          return NULL;
  

•  return blob;
  

+}

+void *bloblist_get_blob(uint tag, int *sizep) { struct bloblist_rec *rec;

  rec = bloblist_findrec(tag);
  if (!rec)
          return NULL;

•  if (size && size != rec->size)
  

•          return NULL;
  

•  *sizep = rec->size;
  
  return (void *)rec + rec_hdr_size(rec);
  

} diff --git a/include/bloblist.h b/include/bloblist.h index f999391f74..52ba0ddcf8 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -250,6 +250,24 @@ static inline void *bloblist_check_magic(ulong addr) return ptr; }

+#if CONFIG_IS_ENABLED(BLOBLIST)

Why should we use an #ifdef here? Why would a caller invoke the function if the configuration is disabled?

Adding #ifdef here is to have the inline function so that we don't need to

add the macro to all of its callers.

...

+/**

• • bloblist_get_blob() - Find a blob and get the size of it
• • Searches the bloblist and returns the blob with the matching tag
• • @tag: Tag to search for (enum bloblist_tag_t)
• • @sizep: Size of the blob found
• • Return: pointer to bloblist if found, or NULL if not found
• */

+void *bloblist_get_blob(uint tag, int *sizep);

If tag is expected to be a value from the enum, we should not use uint here.

This is just following the original convention of existing bloblist

functions which are all using uint for the tag.

Regards, Raymond

...

+#else +static inline void *bloblist_get_blob(uint tag, int *sizep) +{

•  return NULL;
  

+} +#endif

/**

• bloblist_find() - Find a blob

diff --git a/test/common/bloblist.c b/test/common/bloblist.c index 4bca62110a..324127596f 100644 --- a/test/common/bloblist.c +++ b/test/common/bloblist.c @@ -98,10 +98,12 @@ static int bloblist_test_blob(struct unit_test_state

*uts)

...

  struct bloblist_hdr *hdr;
  struct bloblist_rec *rec, *rec2;
  char *data;

•  int size = 0;
  
  /* At the start there should be no records */
  hdr = clear_bloblist();
  ut_assertnull(bloblist_find(TEST_TAG, TEST_BLOBLIST_SIZE));
  

•  ut_assertnull(bloblist_get_blob(TEST_TAG, &size));
  ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0, 0));
  ut_asserteq(sizeof(struct bloblist_hdr), bloblist_get_size());
  ut_asserteq(TEST_BLOBLIST_SIZE, bloblist_get_total_size());
  

@@ -114,6 +116,8 @@ static int bloblist_test_blob(struct unit_test_state

*uts)

...

  ut_asserteq_addr(rec + 1, data);
  data = bloblist_find(TEST_TAG, TEST_SIZE);
  ut_asserteq_addr(rec + 1, data);

•  ut_asserteq_addr(bloblist_get_blob(TEST_TAG, &size), data);
  

•  ut_asserteq(size, TEST_SIZE);
  
  /* Check the data is zeroed */
  ut_assertok(check_zero(data, TEST_SIZE));
  

Hi Heinrich,

On Mon, 13 Jan 2025 at 12:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 13.01.25 15:33, Raymond Mao wrote:

...

Hi Heinrich,

On Fri, 10 Jan 2025 at 19:04, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao
<raymond.mao@linaro.org <mailto:raymond.mao@linaro.org>>:
 >bloblist_find function only returns the pointer of blob data,
 >which is fine for those self-describing data like FDT.
 >But as a common scenario, an interface is needed to retrieve both
 >the pointer and the size of the blob data.
 >
 >Add a few ut test cases for the new api.
 >
 >Signed-off-by: Raymond Mao <raymond.mao@linaro.org
<mailto:raymond.mao@linaro.org>>
 >Reviewed-by: Simon Glass <sjg@chromium.org <mailto:

sjg@chromium.org>>

...

 >Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org
<mailto:ilias.apalodimas@linaro.org>>
 >---
 >Changes in v2
 >- Rename function argument.
 >- Add ut tests.
 >Changes in v3
 >- None.
 >Changes in v4
 >- None.
 >
 > common/bloblist.c      | 17 +++++++++++++++--
 > include/bloblist.h     | 18 ++++++++++++++++++
 > test/common/bloblist.c |  4 ++++
 > 3 files changed, 37 insertions(+), 2 deletions(-)
 >
 >diff --git a/common/bloblist.c b/common/bloblist.c
 >index 110bb9dc44..ab48a3cdb9 100644
 >--- a/common/bloblist.c
 >+++ b/common/bloblist.c
 >@@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag,
struct bloblist_rec **recp, int size,
 > }
 >
 > void *bloblist_find(uint tag, int size)
 >+{
 >+      void *blob = NULL;
 >+      int blob_size;
 >+
 >+      blob = bloblist_get_blob(tag, &blob_size);
 >+
 >+      if (size && size != blob_size)
 >+              return NULL;
 >+
 >+      return blob;
 >+}
 >+
 >+void *bloblist_get_blob(uint tag, int *sizep)
 > {
 >       struct bloblist_rec *rec;
 >
 >       rec = bloblist_findrec(tag);
 >       if (!rec)
 >               return NULL;
 >-      if (size && size != rec->size)
 >-              return NULL;
 >+
 >+      *sizep = rec->size;
 >
 >       return (void *)rec + rec_hdr_size(rec);
 > }
 >diff --git a/include/bloblist.h b/include/bloblist.h
 >index f999391f74..52ba0ddcf8 100644
 >--- a/include/bloblist.h
 >+++ b/include/bloblist.h
 >@@ -250,6 +250,24 @@ static inline void
*bloblist_check_magic(ulong addr)
 >       return ptr;
 > }
 >
 >+#if CONFIG_IS_ENABLED(BLOBLIST)

Why should we use an #ifdef here?
Why would a caller invoke the function if the configuration is

disabled?

...

Adding #ifdef here is to have the inline function so that we don't need to add the macro to all of its callers.

There is just one. See patch 3, where you already check the CONFIG albeit in the wrong place.

Currently it is only one for TPM eventlog, but according to the Firmware Handoff spec we will use Transfer List (aka bloblist in U-Boot) to handover different information (blobs) from previous boot stage, so that we will need to call bloblist_get_blob in multiple subsystems in the near future.

In patch #3, checking BLOBLIST is a temporary solution since we don't have a good idea at the moment on a required config to enable/disable "handoff from previous boot stage using bloblist" - which is separate from BLOBLIST itself. As discussed with Tom, I use BLOBLIST as a temporary solution but this will be replaced in the future.

See my reply in patch #3 for more information.

Regards, Raymond

...

 >+/**
 >+ * bloblist_get_blob() - Find a blob and get the size of it
 >+ *
 >+ * Searches the bloblist and returns the blob with the matching

tag

...

 >+ *
 >+ * @tag:      Tag to search for (enum bloblist_tag_t)
 >+ * @sizep:    Size of the blob found
 >+ * Return: pointer to bloblist if found, or NULL if not found
 >+ */
 >+void *bloblist_get_blob(uint tag, int *sizep);

If tag is expected to be a value from the enum, we should not use
uint here.

This is just following the original convention of existing bloblist functions which are all using uint for the tag.

@Simon: Why aren't we using enum?

Best regards

Heinrich

...

Regards, Raymond

 >+#else
 >+static inline void *bloblist_get_blob(uint tag, int *sizep)
 >+{
 >+      return NULL;
 >+}
 >+#endif
 >+
 > /**
 >  * bloblist_find() - Find a blob
 >  *
 >diff --git a/test/common/bloblist.c b/test/common/bloblist.c
 >index 4bca62110a..324127596f 100644
 >--- a/test/common/bloblist.c
 >+++ b/test/common/bloblist.c
 >@@ -98,10 +98,12 @@ static int bloblist_test_blob(struct
unit_test_state *uts)
 >       struct bloblist_hdr *hdr;
 >       struct bloblist_rec *rec, *rec2;
 >       char *data;
 >+      int size = 0;
 >
 >       /* At the start there should be no records */
 >       hdr = clear_bloblist();
 >       ut_assertnull(bloblist_find(TEST_TAG, TEST_BLOBLIST_SIZE));
 >+      ut_assertnull(bloblist_get_blob(TEST_TAG, &size));
 >       ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0,

0));

...

 >       ut_asserteq(sizeof(struct bloblist_hdr),

bloblist_get_size());

...

 >       ut_asserteq(TEST_BLOBLIST_SIZE, bloblist_get_total_size());
 >@@ -114,6 +116,8 @@ static int bloblist_test_blob(struct
unit_test_state *uts)
 >       ut_asserteq_addr(rec + 1, data);
 >       data = bloblist_find(TEST_TAG, TEST_SIZE);
 >       ut_asserteq_addr(rec + 1, data);
 >+      ut_asserteq_addr(bloblist_get_blob(TEST_TAG, &size), data);
 >+      ut_asserteq(size, TEST_SIZE);
 >
 >       /* Check the data is zeroed */
 >       ut_assertok(check_zero(data, TEST_SIZE));