Skip to contentSkip to conversation

efi_loader: Make RTS relocation more robust

PATCH Superseded Alexander Graf 2 participants 2 messages Dec 9, 2018 – Dec 10, 2018

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 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")
---
 lib/efi_loader/efi_runtime.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
lib/efi_loader/efi_runtime.c +19 -3
index 95844efdb0..a9e94f78c3 100644
@@ -436,14 +436,30 @@ 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);
 
+	/* Ensure we see exactly one single runtime section */
+	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 +499,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
Signature · 1 line
2.12.3
#1 lore raw

PATCH 1/1 efi_loader: Make RTS relocation more robust

Quoted 47 lines · Alexander Graf

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")
---
 lib/efi_loader/efi_runtime.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
lib/efi_loader/efi_runtime.c +8 -2
index 95844efdb0..a9e94f78c3 100644
@@ -436,14 +436,30 @@ 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);
 
+	/* Ensure we see exactly one single runtime section */
+	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++;

lib/efi_loader/efi_runtime.c static efi_status_t EFIAPI efi_set_virtual_address_map( · line 462

Quoted 10 lines
lib/efi_loader/efi_runtime.c +10 -0
+	}
+
+	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);
+	}
+

This does not match the UEFI specification. We can use the bootefi command to load runtime drivers. But unfortunately we do not serve the registered events in SetVirtualAddressMap() and have not implemented ConvertPointer() yet.

Please mention this deficiency in the comments as a TODO.

Tested on Odroid C2.

Quoted 13 lines
lib/efi_loader/efi_runtime.c +1 -1
 	/* Rebind mmio pointers */
 	for (i = 0; i < n; i++) {
 		struct efi_mem_desc *map = (void*)virtmap +
@@ -483,7 +499,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