efi_loader: Make RTS relocation more robust
PATCH v2 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.
---
v1 -> v2:
- Add more verbose comment explaining why we have the sanity check.
--- lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)
lib/efi_loader/efi_runtime.c +31 -3
@@ -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
Signature · 1 line
2.12.3
PATCH v2 1/1 efi_loader: Make RTS relocation more robust
Quoted 15 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_CODEsection 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.
Quoted 4 lines
Fixes 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")Reported-by Heinrich Schuchardt <[email protected]>Reported-by Loic Devulder <[email protected]>Signed-off-by Alexander Graf <[email protected]>
Quoted 70 lines
---
v1 -> v2:
- Add more verbose comment explaining why we have the sanity check.
--- lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)lib/efi_loader/efi_runtime.c +31
-3@@ -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
PATCH v2 1/1 efi_loader: Make RTS relocation more robust
In reply to Alexander GrafHi Alex,
Tested successfully on Khadas VIM board, fix my panic with efivars.
Quoted 89 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_CODEsection 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 <[email protected]>Reported-by Loic Devulder <[email protected]>Signed-off-by Alexander Graf <[email protected]>---
v1 -> v2:
- Add more verbose comment explaining why we have the sanity check.
--- lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)lib/efi_loader/efi_runtime.c +31
-3@@ -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
Signature · 7 lines
Loic Devulder <[email protected]> | ldevulder at irc 0x175A963893C85F55 | D220 DEF5 56A3 DE00 9DAA 78BA 175A 9638 93C8 5F55 Senior QA Engineer | Container & Storage Solutions Quality Assurance team (qa-css) SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB, 21284 (AG Nuernberg)
PATCH v2 1/1 efi_loader: Make RTS relocation more robust
In reply to Alexander GrafQuoted 15 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_CODEsection 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.
Quoted 4 lines
Fixes 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")Reported-by Heinrich Schuchardt <[email protected]>Reported-by Loic Devulder <[email protected]>Signed-off-by Alexander Graf <[email protected]>
This fixes booting OpenBSD on qemu-system-aarch64 where previously it would drop into the kernel debugger when probing efi runtime services.
Quoted 76 lines
---
v1 -> v2:
- Add more verbose comment explaining why we have the sanity check.
--- lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)lib/efi_loader/efi_runtime.c +31
-3@@ -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 Signature · 6 lines
2.12.3 _______________________________________________ U-Boot mailing list U-Boot at lists.denx.de https://lists.denx.de/listinfo/u-boot
Alexander Graf
AppliedPATCH v2 1/1 efi_loader: Make RTS relocation more robust
In reply to Alexander GrafQuoted 15 lines
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_CODEsection 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.
Quoted 7 lines
Fixes 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")Reported-by Heinrich Schuchardt <[email protected]>Reported-by Loic Devulder <[email protected]>Signed-off-by Alexander Graf <[email protected]>Reviewed-by Heinrich Schuchardt <[email protected]>Tested-by Loic Devulder <[email protected]>Tested-by Jonathan Gray <[email protected]>
Thanks, applied to efi-2019.01
Alex