From 05aceb2b1c9d88aafcb8dfbedb24742a24d986ba Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 22 Dec 2018 00:37:41 +0100 Subject: [PATCH 1/3] efi_selftest: block device: avoid read after free Reading the position in a file after closing the same results in a read after free. Correct the sequence in the test. Reported-by: Marek Vasut Signed-off-by: Heinrich Schuchardt Signed-off-by: Alexander Graf --- lib/efi_selftest/efi_selftest_block_device.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index d4e4fac1c7..f038da9f19 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -445,11 +445,6 @@ static int execute(void) efi_st_error("Failed to write file\n"); return EFI_ST_FAILURE; } - ret = file->close(file); - if (ret != EFI_SUCCESS) { - efi_st_error("Failed to close file\n"); - return EFI_ST_FAILURE; - } ret = file->getpos(file, &pos); if (ret != EFI_SUCCESS) { efi_st_error("GetPosition failed\n"); @@ -460,6 +455,11 @@ static int execute(void) (unsigned int)pos); return EFI_ST_FAILURE; } + ret = file->close(file); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to close file\n"); + return EFI_ST_FAILURE; + } /* Verify file */ boottime->set_mem(buf, sizeof(buf), 0); From 5bdb0a7cad05df35cd87ebce683eda032a8abc87 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 18 Dec 2018 18:06:14 +0100 Subject: [PATCH 2/3] lib: crc32: mark static variable as __efi_runtime_data In commit 483dbab9f931 ("lib: crc32: mark function crc32() as __efi_runtime") one local static variable was missed. It did not end up in the __efi_runtime_data section as it should. If CONFIG_DYNAMIC_CRC_TABLE=y a data abort execption may occur when the UEFI payload calls the SetVirtualAddressMap() runtime service. Reported-by: Dominik Adamski Fixes: 483dbab9f931 ("lib: crc32: mark function crc32() as __efi_runtime") Signed-off-by: Heinrich Schuchardt Signed-off-by: Alexander Graf --- lib/crc32.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/crc32.c b/lib/crc32.c index 71e27df78e..eee21f8d73 100644 --- a/lib/crc32.c +++ b/lib/crc32.c @@ -65,7 +65,8 @@ static void __efi_runtime make_crc_table(void) int n, k; uLong poly; /* polynomial exclusive-or pattern */ /* terms of polynomial defining this crc (except x^32): */ - static const Byte p[] = {0,1,2,4,5,7,8,10,11,12,16,22,23,26}; + static Byte __efi_runtime_data p[] = { + 0, 1, 2, 4, 5, 7, 8, 10, 11, 12, 16, 22, 23, 26}; /* make exclusive-or pattern from polynomial (0xedb88320L) */ poly = 0L; From 5c38e05ed8ce468585b3f4aceb4ebf37b904d3f1 Mon Sep 17 00:00:00 2001 From: Alexander Graf Date: Tue, 11 Dec 2018 10:00:42 +0100 Subject: [PATCH 3/3] efi_loader: Make RTS relocation more robust While changing the RTS alignment to 64KB in commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb") the relocation code started to break. The reason for that is that we didn't actually look at the real relocation data. We merely took the RUNTIME_CODE section as a hint and started to relocate based on self calculated data from that point on. That calculation was now out of sync though. To ensure we're not running into such a situation again, this patch makes the runtime relocation code a bit more robust. We can just trust the phys/virt hints from the payload. We also should check that we really only have a single section, as the code doesn't handle multiple code relocations yet. Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb") Reported-by: Heinrich Schuchardt Reported-by: Loic Devulder Signed-off-by: Alexander Graf Reviewed-by: Heinrich Schuchardt Tested-by: Loic Devulder Tested-by: Jonathan Gray Signed-off-by: Alexander Graf --- lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 95844efdb0..fff93f0960 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -436,14 +436,42 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( uint32_t descriptor_version, struct efi_mem_desc *virtmap) { - ulong runtime_start = (ulong)&__efi_runtime_start & - ~(ulong)EFI_PAGE_MASK; int n = memory_map_size / descriptor_size; int i; + int rt_code_sections = 0; EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size, descriptor_version, virtmap); + /* + * TODO: + * Further down we are cheating. While really we should implement + * SetVirtualAddressMap() events and ConvertPointer() to allow + * dynamically loaded drivers to expose runtime services, we don't + * today. + * + * So let's ensure we see exactly one single runtime section, as + * that is the built-in one. If we see more (or less), someone must + * have tried adding or removing to that which we don't support yet. + * In that case, let's better fail rather than expose broken runtime + * services. + */ + for (i = 0; i < n; i++) { + struct efi_mem_desc *map = (void*)virtmap + + (descriptor_size * i); + + if (map->type == EFI_RUNTIME_SERVICES_CODE) + rt_code_sections++; + } + + if (rt_code_sections != 1) { + /* + * We expose exactly one single runtime code section, so + * something is definitely going wrong. + */ + return EFI_EXIT(EFI_INVALID_PARAMETER); + } + /* Rebind mmio pointers */ for (i = 0; i < n; i++) { struct efi_mem_desc *map = (void*)virtmap + @@ -483,7 +511,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( map = (void*)virtmap + (descriptor_size * i); if (map->type == EFI_RUNTIME_SERVICES_CODE) { ulong new_offset = map->virtual_start - - (runtime_start - gd->relocaddr); + map->physical_start + gd->relocaddr; efi_runtime_relocate(new_offset, map); /* Once we're virtual, we can no longer handle