Re: [PATCH v2 1/1] efi_leader: delete rng-seed if having EFI RNG protocol

[Heinrich Schuchardt]

On 9/17/24 08:38, Ilias Apalodimas wrote:

Hi Heinrich,

On Sat, Sep 14, 2024 at 06:08:12PM +0200, Heinrich Schuchardt wrote:

For measured be boot we must avoid any volatile values in the device-tree.
We already delete /chosen/kaslr-seed if we provide and EFI RNG protocol.

Additionally remove /chosen/rng-seed provided by QEMU or U-Boot.

Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
v2:
        put log_debug() in else branch
---
  include/efi_loader.h          |  2 +-
  lib/efi_loader/efi_dt_fixup.c | 16 +++++++++++-----
  lib/efi_loader/efi_helper.c   |  2 +-
  3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index f84852e384f..511281e150e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -567,7 +567,7 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t 
debug_disposition,
  /* Carve out DT reserved memory ranges */
  void efi_carve_out_dt_rsv(void *fdt);
  /* Purge unused kaslr-seed */
-void efi_try_purge_kaslr_seed(void *fdt);
+void efi_try_purge_rng_seed(void *fdt);
  /* Called by bootefi to make console interface available */
  efi_status_t efi_console_register(void);
  /* Called by efi_init_obj_list() to proble all block devices */
diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c
index 9d017804eea..b97758d1305 100644
--- a/lib/efi_loader/efi_dt_fixup.c
+++ b/lib/efi_loader/efi_dt_fixup.c
@@ -41,7 +41,7 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
  }

  /**
- * efi_try_purge_kaslr_seed() - Remove unused kaslr-seed
+ * efi_try_purge_rng_seed() - Remove unused kaslr-seed, rng-seed
   *
   * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization
   * and completely ignores the kaslr-seed for its own randomness needs
@@ -51,8 +51,9 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
   *
   * @fdt: Pointer to device tree
   */
-void efi_try_purge_kaslr_seed(void *fdt)
+void efi_try_purge_rng_seed(void *fdt)
  {
+       const char * const prop[] = {"kaslr-seed", "rng-seed"};
        const efi_guid_t efi_guid_rng_protocol = EFI_RNG_PROTOCOL_GUID;
        struct efi_handler *handler;
        efi_status_t ret;
@@ -67,9 +68,14 @@ void efi_try_purge_kaslr_seed(void *fdt)
        if (nodeoff < 0)
                return;

-       err = fdt_delprop(fdt, nodeoff, "kaslr-seed");
-       if (err < 0 && err != -FDT_ERR_NOTFOUND)
-               log_err("Error deleting kaslr-seed\n");
+       for (const char * const *pos = prop; pos < &prop[ARRAY_SIZE(prop)];

I think
for (int i = 0; i < ARRAY_SIZE(prop); i++)
     fdt_delprop(fdt, nodeoff, prop[i]);

is a lot easier to read

On RISC-V using an index needs 10 bytes more of binary code.
Easy to read and efficient are conflicting here.

I will respin the patch.

Best regards

Heinrich

Other than that, the patch looks fine

Thanks
/Ilias

+            ++pos) {
+               err = fdt_delprop(fdt, nodeoff, *pos);
+               if (err < 0 && err != -FDT_ERR_NOTFOUND)
+                       log_err("Error deleting %s\n", *pos);
+               else
+                       log_debug("Deleted /chosen/%s\n", *pos);
+       }
  }

  /**
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 96f847652ec..a481eb4b7e3 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -522,7 +522,7 @@ efi_status_t efi_install_fdt(void *fdt)
        /* Create memory reservations as indicated by the device tree */
        efi_carve_out_dt_rsv(fdt);

-       efi_try_purge_kaslr_seed(fdt);
+       efi_try_purge_rng_seed(fdt);

        if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
                ret = efi_tcg2_measure_dtb(fdt);
--
2.45.2