* [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust
@ 2018-12-11 9:00 Alexander Graf
2018-12-11 11:52 ` Heinrich Schuchardt
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Alexander Graf @ 2018-12-11 9:00 UTC (permalink / raw)
To: u-boot
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 <xypron.glpk@gmx.de>
Reported-by: Loic Devulder <ldevulder@suse.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
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(-)
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
--
2.12.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust
2018-12-11 9:00 [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust Alexander Graf
@ 2018-12-11 11:52 ` Heinrich Schuchardt
2018-12-11 16:13 ` Loic Devulder
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2018-12-11 11:52 UTC (permalink / raw)
To: u-boot
On 12/11/18 10:00 AM, Alexander Graf wrote:
> 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 <xypron.glpk@gmx.de>
> Reported-by: Loic Devulder <ldevulder@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> ---
>
> 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(-)
>
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust
2018-12-11 9:00 [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust Alexander Graf
2018-12-11 11:52 ` Heinrich Schuchardt
@ 2018-12-11 16:13 ` Loic Devulder
2018-12-14 1:15 ` Jonathan Gray
2018-12-23 3:02 ` [U-Boot] [U-Boot, " Alexander Graf
3 siblings, 0 replies; 5+ messages in thread
From: Loic Devulder @ 2018-12-11 16:13 UTC (permalink / raw)
To: u-boot
Hi Alex,
Tested successfully on Khadas VIM board, fix my panic with efivars.
Tested-by: Loic Devulder <ldevulder@suse.de>
On 12/11/18 10:00 AM, Alexander Graf wrote:
> 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 <xypron.glpk@gmx.de>
> Reported-by: Loic Devulder <ldevulder@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> 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(-)
>
> 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
>
--
Loic Devulder <ldevulder@suse.com> | 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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust
2018-12-11 9:00 [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust Alexander Graf
2018-12-11 11:52 ` Heinrich Schuchardt
2018-12-11 16:13 ` Loic Devulder
@ 2018-12-14 1:15 ` Jonathan Gray
2018-12-23 3:02 ` [U-Boot] [U-Boot, " Alexander Graf
3 siblings, 0 replies; 5+ messages in thread
From: Jonathan Gray @ 2018-12-14 1:15 UTC (permalink / raw)
To: u-boot
On Tue, Dec 11, 2018 at 10:00:42AM +0100, Alexander Graf wrote:
> 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 <xypron.glpk@gmx.de>
> Reported-by: Loic Devulder <ldevulder@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
This fixes booting OpenBSD on qemu-system-aarch64 where previously it
would drop into the kernel debugger when probing efi runtime services.
Tested-by: Jonathan Gray <jsg@jsg.id.au>
>
> ---
>
> 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(-)
>
> 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
> --
> 2.12.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [U-Boot, v2] efi_loader: Make RTS relocation more robust
2018-12-11 9:00 [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust Alexander Graf
` (2 preceding siblings ...)
2018-12-14 1:15 ` Jonathan Gray
@ 2018-12-23 3:02 ` Alexander Graf
3 siblings, 0 replies; 5+ messages in thread
From: Alexander Graf @ 2018-12-23 3:02 UTC (permalink / raw)
To: u-boot
> 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 <xypron.glpk@gmx.de>
> Reported-by: Loic Devulder <ldevulder@suse.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Tested-by: Loic Devulder <ldevulder@suse.de>
> Tested-by: Jonathan Gray <jsg@jsg.id.au>
Thanks, applied to efi-2019.01
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-23 3:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-11 9:00 [U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust Alexander Graf
2018-12-11 11:52 ` Heinrich Schuchardt
2018-12-11 16:13 ` Loic Devulder
2018-12-14 1:15 ` Jonathan Gray
2018-12-23 3:02 ` [U-Boot] [U-Boot, " Alexander Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox