[U-Boot] [PATCH v3 0/6] efi_loader: correctly initialize system table crc32

[U-Boot] [PATCH v3 0/6] efi_loader: correctly initialize system table crc32

[Heinrich Schuchardt]

The crc32 of the system table has to be recalculated when
InstallConfigurationTable() changes the number of tables.

Adjust the signature of CalculateCrc32().

Provide a unit test that checks the crc32 of the system table, the runtime
services table, and the boot sevices table before and after
ExitBootServices().

Enhance the unit test for InstallConfigurationTable() to check the crc32.

Update the crc32 of the runtime services table when detaching. 

Tested with Tracis CI:
https://travis-ci.org/xypron2/u-boot/builds/401158750

v3:
	Update the crc32 of the runtime services table when detaching.
	Adjust the signature of CalculateCrc32().
	Recalculate crc32 in InstallConfigurationTable() and unit test.
v2:
	avoid a warning in a debug statement


*** BLURB HERE ***

Heinrich Schuchardt (6):
  efi_loader: correct signature of CalculateCrc32()
  efi_loader: update crc32 in InstallConfigurationTable
  efi_selftest: check crc32 for InstallConfigurationTable
  efi_selftest: unit test for CalculateCrc32()
  lib: crc32: mark function crc32() as __efi_runtime
  efi_loader: update runtime services table crc32

 include/efi_api.h                            |   5 +-
 include/efi_loader.h                         |   3 +
 lib/crc32.c                                  |  14 +-
 lib/efi_loader/efi_boottime.c                |  23 +--
 lib/efi_loader/efi_runtime.c                 |  15 ++
 lib/efi_selftest/Makefile                    |   1 +
 lib/efi_selftest/efi_selftest_config_table.c |  43 ++++++
 lib/efi_selftest/efi_selftest_crc32.c        | 141 +++++++++++++++++++
 8 files changed, 222 insertions(+), 23 deletions(-)
 create mode 100644 lib/efi_selftest/efi_selftest_crc32.c

-- 
2.18.0

[U-Boot] [PATCH v3 1/6] efi_loader: correct signature of CalculateCrc32()

[Heinrich Schuchardt]

Use const for the buffer. We are not changing the buffer.
Use efi_uintn_t where prescribed by the UEFI spec.
Prefer u32 over uint32_t.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3
	no change
---
 include/efi_api.h             | 5 +++--
 lib/efi_loader/efi_boottime.c | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index c98cc34908..ebf2a3bc18 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -165,8 +165,9 @@ struct efi_boot_services {
 			void **handle, ...);
 	efi_status_t (EFIAPI *uninstall_multiple_protocol_interfaces)(
 			void *handle, ...);
-	efi_status_t (EFIAPI *calculate_crc32)(void *data,
-			unsigned long data_size, uint32_t *crc32);
+	efi_status_t (EFIAPI *calculate_crc32)(const void *data,
+					       efi_uintn_t data_size,
+					       u32 *crc32);
 	void (EFIAPI *copy_mem)(void *destination, const void *source,
 			size_t length);
 	void (EFIAPI *set_mem)(void *buffer, size_t size, uint8_t value);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 9ac8e18680..3689cebf8f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2418,11 +2418,11 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
  * @crc32_p:		cyclic redundancy code
  * Return Value:	status code
  */
-static efi_status_t EFIAPI efi_calculate_crc32(void *data,
-					       unsigned long data_size,
-					       uint32_t *crc32_p)
+static efi_status_t EFIAPI efi_calculate_crc32(const void *data,
+					       efi_uintn_t data_size,
+					       u32 *crc32_p)
 {
-	EFI_ENTRY("%p, %ld", data, data_size);
+	EFI_ENTRY("%p, %zu", data, data_size);
 	*crc32_p = crc32(0, data, data_size);
 	return EFI_EXIT(EFI_SUCCESS);
 }
-- 
2.18.0

[U-Boot] [PATCH v3 2/6] efi_loader: update crc32 in InstallConfigurationTable

[Heinrich Schuchardt]

If the number of installed tables is changed in
InstallConfigurationTable() update the crc32 of the system table.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3:
	new patch
---
 lib/efi_loader/efi_boottime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 3689cebf8f..ea1e4953dc 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1429,6 +1429,9 @@ efi_status_t efi_install_configuration_table(const efi_guid_t *guid,
 	systab.nr_tables = i + 1;
 
 out:
+	/* systab.nr_tables may have changed. So we need to update the crc32 */
+	efi_update_table_header_crc32(&systab.hdr);
+
 	/* Notify that the configuration table was changed */
 	list_for_each_entry(evt, &efi_events, link) {
 		if (evt->group && !guidcmp(evt->group, guid)) {
-- 
2.18.0

[U-Boot] [PATCH v3 3/6] efi_selftest: check crc32 for InstallConfigurationTable

[Heinrich Schuchardt]

InstallConfigurationTable() may change the number of installed
configuration tables.

Check the crc32 of the system table.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3
	new patch
---
 lib/efi_selftest/efi_selftest_config_table.c | 43 ++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/efi_selftest/efi_selftest_config_table.c b/lib/efi_selftest/efi_selftest_config_table.c
index 627b73cdf8..2aa3fc7284 100644
--- a/lib/efi_selftest/efi_selftest_config_table.c
+++ b/lib/efi_selftest/efi_selftest_config_table.c
@@ -32,6 +32,36 @@ static void EFIAPI notify(struct efi_event *event, void *context)
 		++*count;
 }
 
+/*
+ * Check crc32 of a table.
+ */
+static int check_table(const void *table)
+{
+	efi_status_t ret;
+	u32 crc32, res;
+	/* Casting from const to not const */
+	struct efi_table_hdr *hdr = (struct efi_table_hdr *)table;
+
+	crc32 = hdr->crc32;
+	/*
+	 * Setting the crc32 of the 'const' table to zero is easier than
+	 * copying
+	 */
+	hdr->crc32 = 0;
+	ret = boottime->calculate_crc32(table, hdr->headersize, &res);
+	/* Reset table crc32 so it stays constant */
+	hdr->crc32 = crc32;
+	if (ret != EFI_ST_SUCCESS) {
+		efi_st_error("CalculateCrc32 failed\n");
+		return EFI_ST_FAILURE;
+	}
+	if (res != crc32) {
+		efi_st_error("Incorrect CRC32\n");
+		return EFI_ST_FAILURE;
+	}
+	return EFI_ST_SUCCESS;
+}
+
 /*
  * Setup unit test.
  *
@@ -135,6 +165,11 @@ static int execute(void)
 		efi_st_error("Incorrect table address\n");
 		return EFI_ST_FAILURE;
 	}
+	if (check_table(sys_table) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking system table\n");
+		return EFI_ST_FAILURE;
+	}
+
 	/* Update table */
 	ret = boottime->install_configuration_table(&table_guid,
 						    (void *)&tables[1]);
@@ -175,6 +210,10 @@ static int execute(void)
 		efi_st_error("Incorrect table address\n");
 		return EFI_ST_FAILURE;
 	}
+	if (check_table(sys_table) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking system table\n");
+		return EFI_ST_FAILURE;
+	}
 
 	/* Delete table */
 	ret = boottime->install_configuration_table(&table_guid, NULL);
@@ -211,6 +250,10 @@ static int execute(void)
 		efi_st_error("Failed to close event\n");
 		return EFI_ST_FAILURE;
 	}
+	if (check_table(sys_table) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking system table\n");
+		return EFI_ST_FAILURE;
+	}
 
 	return EFI_ST_SUCCESS;
 }
-- 
2.18.0

[U-Boot] [PATCH v3 4/6] efi_selftest: unit test for CalculateCrc32()

[Heinrich Schuchardt]

This unit test checks the CalculateCrc32 bootservice and checks the
headers of the system table, the boot services tablle, and the runtime
services table before and after ExitBootServices().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3
	no change
v2
	no change
---
 lib/efi_selftest/Makefile             |   1 +
 lib/efi_selftest/efi_selftest_crc32.c | 141 ++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)
 create mode 100644 lib/efi_selftest/efi_selftest_crc32.c

diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index c0fd490d2e..1f3084cfb8 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -16,6 +16,7 @@ efi_selftest_bitblt.o \
 efi_selftest_config_table.o \
 efi_selftest_controllers.o \
 efi_selftest_console.o \
+efi_selftest_crc32.o \
 efi_selftest_devicepath.o \
 efi_selftest_devicepath_util.o \
 efi_selftest_events.o \
diff --git a/lib/efi_selftest/efi_selftest_crc32.c b/lib/efi_selftest/efi_selftest_crc32.c
new file mode 100644
index 0000000000..8555b8f114
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_crc32.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * efi_selftest_crc32
+ *
+ * Copyright (c) 2018 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * This unit test checks the CalculateCrc32 bootservice and checks the
+ * headers of the system table, the boot services tablle, and the runtime
+ * services table before and after ExitBootServices().
+ */
+
+#include <efi_selftest.h>
+
+const struct efi_system_table *st;
+efi_status_t (EFIAPI *bs_crc32)(const void *data, efi_uintn_t data_size,
+				u32 *crc32);
+
+static int check_table(const void *table)
+{
+	efi_status_t ret;
+	u32 crc32, res;
+	/* Casting from const to not const */
+	struct efi_table_hdr *hdr = (struct efi_table_hdr *)table;
+
+	if (!hdr->signature) {
+		efi_st_error("Missing header signature\n");
+		return EFI_ST_FAILURE;
+	}
+	if (!hdr->revision) {
+		efi_st_error("Missing header revision\n");
+		return EFI_ST_FAILURE;
+	}
+	if (hdr->headersize <= sizeof(struct efi_table_hdr)) {
+		efi_st_error("Incorrect headersize value\n");
+		return EFI_ST_FAILURE;
+	}
+	if (hdr->reserved) {
+		efi_st_error("Reserved header field is not zero\n");
+		return EFI_ST_FAILURE;
+	}
+
+	crc32 = hdr->crc32;
+	/*
+	 * Setting the crc32 of the 'const' table to zero is easier than
+	 * copying
+	 */
+	hdr->crc32 = 0;
+	ret = bs_crc32(table, hdr->headersize, &res);
+	/* Reset table crc32 so it stays constant */
+	hdr->crc32 = crc32;
+	if (ret != EFI_ST_SUCCESS) {
+		efi_st_error("CalculateCrc32 failed\n");
+		return EFI_ST_FAILURE;
+	}
+	if (res != crc32) {
+		efi_st_error("Incorrect CRC32\n");
+		// return EFI_ST_FAILURE;
+	}
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Setup unit test.
+ *
+ * Check that CalculateCrc32 is working correctly.
+ * Check tables before ExitBootServices().
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	efi_status_t ret;
+	u32 res;
+
+	st = systable;
+	bs_crc32 = systable->boottime->calculate_crc32;
+
+	/* Check that CalculateCrc32 is working */
+	ret = bs_crc32("U-Boot", 6, &res);
+	if (ret != EFI_ST_SUCCESS) {
+		efi_st_error("CalculateCrc32 failed\n");
+		return EFI_ST_FAILURE;
+	}
+	if (res != 0x134b0db4) {
+		efi_st_error("Incorrect CRC32\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Check tables before ExitBootServices() */
+	if (check_table(st) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking system table\n");
+		return EFI_ST_FAILURE;
+	}
+	if (check_table(st->boottime) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking boottime table\n");
+		return EFI_ST_FAILURE;
+	}
+	if (check_table(st->runtime) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking runtime table\n");
+		return EFI_ST_FAILURE;
+	}
+
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Execute unit test
+ *
+ * Check tables after ExitBootServices()
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int execute(void)
+{
+	if (check_table(st) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking system table\n");
+		return EFI_ST_FAILURE;
+	}
+	if (check_table(st->runtime) != EFI_ST_SUCCESS) {
+		efi_st_error("Checking runtime table\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/*
+	 * We cannot call SetVirtualAddressMap() and recheck the runtime
+	 * table afterwards because this would invalidate the addresses of the
+	 * unit tests.
+	 */
+
+	return EFI_ST_SUCCESS;
+}
+
+EFI_UNIT_TEST(crc32) = {
+	.name = "crc32",
+	.phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+};
-- 
2.18.0

[U-Boot] [PATCH v3 5/6] lib: crc32: mark function crc32() as __efi_runtime

[Heinrich Schuchardt]

The function crc32() is needed by the EFI subsystem at runtime. So it has
to be linked into the runtime section together with all dependencies.

Eliminate empty define ZEXPORT.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3
	new patch
---
 lib/crc32.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/crc32.c b/lib/crc32.c
index 7f545fde4a..bcb140ba06 100644
--- a/lib/crc32.c
+++ b/lib/crc32.c
@@ -12,6 +12,7 @@
 #include <arpa/inet.h>
 #else
 #include <common.h>
+#include <efi_loader.h>
 #endif
 #include <compiler.h>
 #include <u-boot/crc.h>
@@ -21,8 +22,11 @@
 #endif
 #include "u-boot/zlib.h"
 
-#define local static
-#define ZEXPORT	/* empty */
+#ifdef USE_HOSTCC
+#define __efi_runtime
+#define __efi_runtime_data
+#endif
+#define local static __efi_runtime_data
 
 #define tole(x) cpu_to_le32(x)
 
@@ -176,7 +180,7 @@ const uint32_t * ZEXPORT get_crc_table()
 /* No ones complement version. JFFS2 (and other things ?)
  * don't use ones compliment in their CRC calculations.
  */
-uint32_t ZEXPORT crc32_no_comp(uint32_t crc, const Bytef *buf, uInt len)
+uint32_t __efi_runtime crc32_no_comp(uint32_t crc, const Bytef *buf, uInt len)
 {
     const uint32_t *tab = crc_table;
     const uint32_t *b =(const uint32_t *)buf;
@@ -218,7 +222,7 @@ uint32_t ZEXPORT crc32_no_comp(uint32_t crc, const Bytef *buf, uInt len)
 }
 #undef DO_CRC
 
-uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *p, uInt len)
+uint32_t __efi_runtime crc32(uint32_t crc, const Bytef *p, uInt len)
 {
      return crc32_no_comp(crc ^ 0xffffffffL, p, len) ^ 0xffffffffL;
 }
@@ -227,9 +231,8 @@ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *p, uInt len)
  * Calculate the crc32 checksum triggering the watchdog every 'chunk_sz' bytes
  * of input.
  */
-uint32_t ZEXPORT crc32_wd (uint32_t crc,
-			   const unsigned char *buf,
-			   uInt len, uInt chunk_sz)
+uint32_t crc32_wd(uint32_t crc, const unsigned char *buf, uInt len,
+		  uInt chunk_sz)
 {
 #if defined(CONFIG_HW_WATCHDOG) || defined(CONFIG_WATCHDOG)
 	const unsigned char *end, *curr;
-- 
2.18.0

[U-Boot] [PATCH v3 6/6] efi_loader: update runtime services table crc32

[Heinrich Schuchardt]

The crc32 of the runtime services table must be updated after detaching.

efi_update_table_header_crc32() must be __efi_runtime. So move it to
efi_runtime.c

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3
	new patch
---
 include/efi_loader.h          |  3 +++
 lib/efi_loader/efi_boottime.c | 12 ------------
 lib/efi_loader/efi_runtime.c  | 15 +++++++++++++++
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 051f9c4514..8363a4c5f4 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -417,6 +417,9 @@ static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
 #define __efi_runtime_data __attribute__ ((section (".data.efi_runtime")))
 #define __efi_runtime __attribute__ ((section (".text.efi_runtime")))
 
+/* Update CRC32 in table header */
+void __efi_runtime efi_update_table_header_crc32(struct efi_table_hdr *table);
+
 /* Call this with mmio_ptr as the _pointer_ to a pointer to an MMIO region
  * to make it available at runtime */
 efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index ea1e4953dc..9ba94e7e01 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -153,18 +153,6 @@ const char *__efi_nesting_dec(void)
 	return indent_string(--nesting_level);
 }
 
-/**
- * efi_update_table_header_crc32() - Update CRC32 in table header
- *
- * @table:	EFI table
- */
-static void efi_update_table_header_crc32(struct efi_table_hdr *table)
-{
-	table->crc32 = 0;
-	table->crc32 = crc32(0, (const unsigned char *)table,
-			     table->headersize);
-}
-
 /**
  * efi_queue_event - queue an EFI event
  *
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 1acb06a206..92175aebdb 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -84,6 +84,18 @@ struct elf_rela {
  * handle a good number of runtime callbacks
  */
 
+/**
+ * efi_update_table_header_crc32() - Update crc32 in table header
+ *
+ * @table:	EFI table
+ */
+void __efi_runtime efi_update_table_header_crc32(struct efi_table_hdr *table)
+{
+	table->crc32 = 0;
+	table->crc32 = crc32(0, (const unsigned char *)table,
+			     table->headersize);
+}
+
 static void EFIAPI efi_reset_system_boottime(
 			enum efi_reset_type reset_type,
 			efi_status_t reset_status,
@@ -249,6 +261,9 @@ static void efi_runtime_detach(ulong offset)
 		debug("%s: Setting %p to %lx\n", __func__, p, newaddr);
 		*p = newaddr;
 	}
+
+	/* Update crc32 */
+	efi_update_table_header_crc32(&efi_runtime_services.hdr);
 }
 
 /* Relocate EFI runtime to uboot_reloc_base = offset */
-- 
2.18.0

[U-Boot] [PATCH v3 5/6] lib: crc32: mark function crc32() as __efi_runtime

[Alexander Graf]

On 07.07.18 15:36, Heinrich Schuchardt wrote:
> The function crc32() is needed by the EFI subsystem at runtime. So it has
> to be linked into the runtime section together with all dependencies.
> 
> Eliminate empty define ZEXPORT.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v3
> 	new patch
> ---
>  lib/crc32.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/crc32.c b/lib/crc32.c
> index 7f545fde4a..bcb140ba06 100644
> --- a/lib/crc32.c
> +++ b/lib/crc32.c
> @@ -12,6 +12,7 @@
>  #include <arpa/inet.h>
>  #else
>  #include <common.h>
> +#include <efi_loader.h>
>  #endif
>  #include <compiler.h>
>  #include <u-boot/crc.h>
> @@ -21,8 +22,11 @@
>  #endif
>  #include "u-boot/zlib.h"
>  
> -#define local static
> -#define ZEXPORT	/* empty */
> +#ifdef USE_HOSTCC
> +#define __efi_runtime
> +#define __efi_runtime_data
> +#endif
> +#define local static __efi_runtime_data

This pushes functions into the data section - which is probably not what
we want. Eventually we may want to have NX protection, so we need to
separate code and data as well as we can.


Alex

[U-Boot] [PATCH v3 6/6] efi_loader: update runtime services table crc32

[Alexander Graf]

On 07.07.18 15:36, Heinrich Schuchardt wrote:
> The crc32 of the runtime services table must be updated after detaching.
> 
> efi_update_table_header_crc32() must be __efi_runtime. So move it to
> efi_runtime.c
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex