[U-Boot] [PATCH v3 0/6] efi_loader: correctly initialize system table crc32
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
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); }
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)) {
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; }
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, +};
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;
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
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 */
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
participants (2)
-
Alexander Graf -
Heinrich Schuchardt