[U-Boot] [PATCH v2 0/3] efi_loader: correctly initialize system table crc32

[U-Boot] [PATCH v2 0/3] efi_loader: correctly initialize system table crc32

[Heinrich Schuchardt]

The crc32 of the system table has to be calculated after all fields have
been set.

Adjust the signature of CalculateCrc32().

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

v2:
	avoid a warning in a debug statement

Heinrich Schuchardt (3):
  efi_loader: correctly initialize system table crc32
  efi_loader: correct signature of CalculateCrc32()
  efi_selftest: unit test for CalculateCrc32()

 cmd/bootefi.c                         |  10 +-
 include/efi_api.h                     |   5 +-
 lib/efi_loader/efi_boottime.c         |   8 +-
 lib/efi_selftest/Makefile             |   1 +
 lib/efi_selftest/efi_selftest_crc32.c | 141 ++++++++++++++++++++++++++
 5 files changed, 154 insertions(+), 11 deletions(-)
 create mode 100644 lib/efi_selftest/efi_selftest_crc32.c

-- 
2.18.0

[U-Boot] [PATCH v2 1/3] efi_loader: correctly initialize system table crc32

[Heinrich Schuchardt]

We should calculate the crc32 after initalizing all fields of the system
table.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	no change
---
 cmd/bootefi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index b60c151fb4..1bf75e2bba 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -44,11 +44,6 @@ efi_status_t efi_init_obj_list(void)
 	if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
 		return efi_obj_list_initialized;
 
-	/* Initialize system table */
-	ret = efi_initialize_system_table();
-	if (ret != EFI_SUCCESS)
-		goto out;
-
 	/* Initialize EFI driver uclass */
 	ret = efi_driver_init();
 	if (ret != EFI_SUCCESS)
@@ -91,6 +86,11 @@ efi_status_t efi_init_obj_list(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
+	/* Initialize system table */
+	ret = efi_initialize_system_table();
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 out:
 	efi_obj_list_initialized = ret;
 	return ret;
-- 
2.18.0

[U-Boot] [PATCH v2 2/3] 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>
---
v2:
	avoid a warning in a debug statement
---
 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 v2 3/3] 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>
---
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 d927208700..590f90b16d 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 v2 3/3] efi_selftest: unit test for CalculateCrc32()

[Alexander Graf]

On 06.07.18 07:09, Heinrich Schuchardt wrote:
> 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>

This is a good step forward, thanks!

I still don't see any code that adapts the systab crc in
efi_install_configuration_table(), so we're clearly missing a test case
for this.


Alex

[U-Boot] [PATCH v2 3/3] efi_selftest: unit test for CalculateCrc32()

[Heinrich Schuchardt]

On 07/06/2018 07:24 AM, Alexander Graf wrote:
> 
> 
> On 06.07.18 07:09, Heinrich Schuchardt wrote:
>> 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>
> 
> This is a good step forward, thanks!
> 
> I still don't see any code that adapts the systab crc in
> efi_install_configuration_table(), so we're clearly missing a test case
> for this.

You are right. We are changing the field NumberOfTableEntries which is
part of the crc32. The field ConfigurationTable is currently not changed.

Best regards

Heinrich

[U-Boot] [U-Boot, v2, 1/3] efi_loader: correctly initialize system table crc32

[Heinrich Schuchardt]

On 07/06/2018 07:22 AM, Alexander Graf wrote:
>> We should calculate the crc32 after initalizing all fields of the system
>> table.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> Thanks, applied to efi-next
> 
> Alex
> 
> 
Hello Alex,

this patch series causes a Travis error. Please, remove it from
efi-next. I will send a new version - after Travis testing.

Best regards

Heinrich

[U-Boot] [U-Boot, v2, 1/3] efi_loader: correctly initialize system table crc32

[Alexander Graf]

On 07.07.18 09:05, Heinrich Schuchardt wrote:
> On 07/06/2018 07:22 AM, Alexander Graf wrote:
>>> We should calculate the crc32 after initalizing all fields of the system
>>> table.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>> Thanks, applied to efi-next
>>
>> Alex
>>
>>
> Hello Alex,
> 
> this patch series causes a Travis error. Please, remove it from
> efi-next. I will send a new version - after Travis testing.

Done.


Thanks,

Alex