[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[Alexander Graf]

While discussing something compeltely different, Ard pointed out
that it might be legal to omit calling SetVirtualAddressMap altogether.

While that sounds great, we currently rely on that call to remove
all function pointers to code that we do not support outside of
boot services.

So let's patch out those bits already on the call to ExitBootServices,
so that we can successfully run even when an OS chooses to omit
any call to SetVirtualAddressMap.

Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 include/efi_loader.h          |  2 ++
 lib/efi_loader/efi_boottime.c |  1 +
 lib/efi_loader/efi_runtime.c  | 27 ++++++++++++++++++---------
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9dd933dae7..2a40b09693 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -310,6 +310,8 @@ void efi_save_gd(void);
 void efi_restore_gd(void);
 /* Call this to relocate the runtime section to an address space */
 void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
+/* Call this when we start to live in a runtime only world */
+void efi_runtime_detach(ulong offset);
 /* Call this to set the current device name */
 void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
 /* Add a new object to the object list. */
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index fc26d6adc1..8cb2979bab 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1917,6 +1917,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	bootm_disable_interrupts();
 
 	/* Disable boot time services */
+	efi_runtime_detach((ulong)gd->relocaddr);
 	systab.con_in_handle = NULL;
 	systab.con_in = NULL;
 	systab.con_out_handle = NULL;
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 636dfdab39..ec7fedba06 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -276,16 +276,12 @@ struct efi_runtime_detach_list_struct {
 	void *patchto;
 };
 
-static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
+static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
 	{
 		/* do_reset is gone */
 		.ptr = &efi_runtime_services.reset_system,
 		.patchto = efi_reset_system,
 	}, {
-		/* invalidate_*cache_all are gone */
-		.ptr = &efi_runtime_services.set_virtual_address_map,
-		.patchto = &efi_unimplemented,
-	}, {
 		/* RTC accessors are gone */
 		.ptr = &efi_runtime_services.get_time,
 		.patchto = &efi_get_time,
@@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p)
 	return false;
 }
 
-static void efi_runtime_detach(ulong offset)
+/**
+ * efi_runtime_detach() - Remove any dependency on non-runtime sections
+ *
+ * This function patches all remaining code to be self-sufficient inside
+ * runtime sections. Any calls to non-runtime will be removed after this.
+ *
+ * @offset:		relocaddr for pre-set_v_a_space, offset to VA after
+ */
+__efi_runtime void efi_runtime_detach(ulong offset)
 {
 	int i;
 	ulong patchoff = offset - (ulong)gd->relocaddr;
@@ -430,19 +434,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
  * @virtmap:		virtual address mapping information
  * Return:		status code
  */
-static efi_status_t EFIAPI efi_set_virtual_address_map(
+static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map(
 			unsigned long memory_map_size,
 			unsigned long descriptor_size,
 			uint32_t descriptor_version,
 			struct efi_mem_desc *virtmap)
 {
+	static __efi_runtime_data bool is_patched;
 	int n = memory_map_size / descriptor_size;
 	int i;
 	int rt_code_sections = 0;
 
+	if (is_patched)
+		return EFI_INVALID_PARAMETER;
+
 	EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size,
 		  descriptor_version, virtmap);
 
+	is_patched = true;
+
 	/*
 	 * TODO:
 	 * Further down we are cheating. While really we should implement
@@ -514,8 +524,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
 					   map->physical_start + gd->relocaddr;
 
 			efi_runtime_relocate(new_offset, map);
-			/* Once we're virtual, we can no longer handle
-			   complex callbacks */
+			/* We need to repatch callbacks for their new VA */
 			efi_runtime_detach(new_offset);
 			return EFI_EXIT(EFI_SUCCESS);
 		}
-- 
2.12.3

[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[Heinrich Schuchardt]

On 1/23/19 6:33 PM, Alexander Graf wrote:
> While discussing something compeltely different, Ard pointed out
> that it might be legal to omit calling SetVirtualAddressMap altogether.
> 
> While that sounds great, we currently rely on that call to remove
> all function pointers to code that we do not support outside of
> boot services.
> 
> So let's patch out those bits already on the call to ExitBootServices,
> so that we can successfully run even when an OS chooses to omit
> any call to SetVirtualAddressMap.

Such an operating system would not be allowed to use any virtual
addresses at any time because runtime drivers would not be informed
about the mapping.

Does such an operating system exist?
Or is this only a hypothetical case?

> 
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Alexander Graf <agraf@suse.de>

I am missing a description of the side effects of the change, e.g.

Our EFI unit tests call the Reset() service.

So Python tests will fail on systems that do not implement Reset() in a
runtime compatible manner.

Best regards

Heinrich


> ---
>  include/efi_loader.h          |  2 ++
>  lib/efi_loader/efi_boottime.c |  1 +
>  lib/efi_loader/efi_runtime.c  | 27 ++++++++++++++++++---------
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9dd933dae7..2a40b09693 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -310,6 +310,8 @@ void efi_save_gd(void);
>  void efi_restore_gd(void);
>  /* Call this to relocate the runtime section to an address space */
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
> +/* Call this when we start to live in a runtime only world */
> +void efi_runtime_detach(ulong offset);
>  /* Call this to set the current device name */
>  void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
>  /* Add a new object to the object list. */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index fc26d6adc1..8cb2979bab 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1917,6 +1917,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>  	bootm_disable_interrupts();
>  
>  	/* Disable boot time services */
> +	efi_runtime_detach((ulong)gd->relocaddr);
>  	systab.con_in_handle = NULL;
>  	systab.con_in = NULL;
>  	systab.con_out_handle = NULL;
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 636dfdab39..ec7fedba06 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -276,16 +276,12 @@ struct efi_runtime_detach_list_struct {
>  	void *patchto;
>  };
>  
> -static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
> +static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
>  	{
>  		/* do_reset is gone */
>  		.ptr = &efi_runtime_services.reset_system,
>  		.patchto = efi_reset_system,
>  	}, {
> -		/* invalidate_*cache_all are gone */
> -		.ptr = &efi_runtime_services.set_virtual_address_map,
> -		.patchto = &efi_unimplemented,
> -	}, {
>  		/* RTC accessors are gone */
>  		.ptr = &efi_runtime_services.get_time,
>  		.patchto = &efi_get_time,
> @@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p)
>  	return false;
>  }
>  
> -static void efi_runtime_detach(ulong offset)
> +/**
> + * efi_runtime_detach() - Remove any dependency on non-runtime sections
> + *
> + * This function patches all remaining code to be self-sufficient inside
> + * runtime sections. Any calls to non-runtime will be removed after this.
> + *
> + * @offset:		relocaddr for pre-set_v_a_space, offset to VA after
> + */
> +__efi_runtime void efi_runtime_detach(ulong offset)
>  {
>  	int i;
>  	ulong patchoff = offset - (ulong)gd->relocaddr;
> @@ -430,19 +434,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
>   * @virtmap:		virtual address mapping information
>   * Return:		status code
>   */
> -static efi_status_t EFIAPI efi_set_virtual_address_map(
> +static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map(
>  			unsigned long memory_map_size,
>  			unsigned long descriptor_size,
>  			uint32_t descriptor_version,
>  			struct efi_mem_desc *virtmap)
>  {
> +	static __efi_runtime_data bool is_patched;
>  	int n = memory_map_size / descriptor_size;
>  	int i;
>  	int rt_code_sections = 0;
>  
> +	if (is_patched)
> +		return EFI_INVALID_PARAMETER;
> +
>  	EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size,
>  		  descriptor_version, virtmap);
>  
> +	is_patched = true;
> +
>  	/*
>  	 * TODO:
>  	 * Further down we are cheating. While really we should implement
> @@ -514,8 +524,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  					   map->physical_start + gd->relocaddr;
>  
>  			efi_runtime_relocate(new_offset, map);
> -			/* Once we're virtual, we can no longer handle
> -			   complex callbacks */
> +			/* We need to repatch callbacks for their new VA */
>  			efi_runtime_detach(new_offset);
>  			return EFI_EXIT(EFI_SUCCESS);
>  		}
> 

[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[Ard Biesheuvel]

On Fri, 25 Jan 2019 at 19:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/23/19 6:33 PM, Alexander Graf wrote:
> > While discussing something compeltely different, Ard pointed out
> > that it might be legal to omit calling SetVirtualAddressMap altogether.
> >
> > While that sounds great, we currently rely on that call to remove
> > all function pointers to code that we do not support outside of
> > boot services.
> >
> > So let's patch out those bits already on the call to ExitBootServices,
> > so that we can successfully run even when an OS chooses to omit
> > any call to SetVirtualAddressMap.
>
> Such an operating system would not be allowed to use any virtual
> addresses at any time because runtime drivers would not be informed
> about the mapping.
>

No. The OS would be permitted to invoke the runtime services at their
original offset.

For arm64, this is trivially achievable, since we already use userland
mappings for the runtime services. On 32-bit architectures, it is more
complicated, since the boot time mapping of peripherals and/or memory
may conflict with the kernel's layout of the address space (e.g., if
your NOR flash lives above 0xc0000000, you *have* to remap it for the
runtime services to be able to use it).



> Does such an operating system exist?
> Or is this only a hypothetical case?
>
> >
> > Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Alexander Graf <agraf@suse.de>
>
> I am missing a description of the side effects of the change, e.g.
>
> Our EFI unit tests call the Reset() service.
>
> So Python tests will fail on systems that do not implement Reset() in a
> runtime compatible manner.
>

All runtime services (except SetVirtualAddresMap() and
ConvertPointer(), obviously) may be used at runtime with or without
installing a virtual address mapping.

[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[Heinrich Schuchardt]

On 1/25/19 10:43 PM, Ard Biesheuvel wrote:
> On Fri, 25 Jan 2019 at 19:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 1/23/19 6:33 PM, Alexander Graf wrote:
>>> While discussing something compeltely different, Ard pointed out
>>> that it might be legal to omit calling SetVirtualAddressMap altogether.
>>>
>>> While that sounds great, we currently rely on that call to remove
>>> all function pointers to code that we do not support outside of
>>> boot services.
>>>
>>> So let's patch out those bits already on the call to ExitBootServices,
>>> so that we can successfully run even when an OS chooses to omit
>>> any call to SetVirtualAddressMap.
>>
>> Such an operating system would not be allowed to use any virtual
>> addresses at any time because runtime drivers would not be informed
>> about the mapping.
>>
> 
> No. The OS would be permitted to invoke the runtime services at their
> original offset.
> 
> For arm64, this is trivially achievable, since we already use userland
> mappings for the runtime services. On 32-bit architectures, it is more
> complicated, since the boot time mapping of peripherals and/or memory
> may conflict with the kernel's layout of the address space (e.g., if
> your NOR flash lives above 0xc0000000, you *have* to remap it for the
> runtime services to be able to use it).
> 
> 
> 
>> Does such an operating system exist?
>> Or is this only a hypothetical case?
>>
>>>
>>> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> I am missing a description of the side effects of the change, e.g.
>>
>> Our EFI unit tests call the Reset() service.
>>
>> So Python tests will fail on systems that do not implement Reset() in a
>> runtime compatible manner.
>>
> 
> All runtime services (except SetVirtualAddresMap() and
> ConvertPointer(), obviously) may be used at runtime with or without
> installing a virtual address mapping.
> 

With this patch in place Travis CI testing fails:

After 'bootefi selftest' this output is not ejected:
m = u_boot_console.p.expect(['resetting', 'U-Boot'])

And after the reset U-Boot crashes (observed for qemu-arm64_defconfig).

Best regards

Heinrich

[U-Boot] [PATCH] efi_loader: Patch non-runtime code out at ExitBootServices already

[Alexander Graf]

On 26.01.19 10:31, Heinrich Schuchardt wrote:
> On 1/25/19 10:43 PM, Ard Biesheuvel wrote:
>> On Fri, 25 Jan 2019 at 19:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> On 1/23/19 6:33 PM, Alexander Graf wrote:
>>>> While discussing something compeltely different, Ard pointed out
>>>> that it might be legal to omit calling SetVirtualAddressMap altogether.
>>>>
>>>> While that sounds great, we currently rely on that call to remove
>>>> all function pointers to code that we do not support outside of
>>>> boot services.
>>>>
>>>> So let's patch out those bits already on the call to ExitBootServices,
>>>> so that we can successfully run even when an OS chooses to omit
>>>> any call to SetVirtualAddressMap.
>>>
>>> Such an operating system would not be allowed to use any virtual
>>> addresses at any time because runtime drivers would not be informed
>>> about the mapping.
>>>
>>
>> No. The OS would be permitted to invoke the runtime services at their
>> original offset.
>>
>> For arm64, this is trivially achievable, since we already use userland
>> mappings for the runtime services. On 32-bit architectures, it is more
>> complicated, since the boot time mapping of peripherals and/or memory
>> may conflict with the kernel's layout of the address space (e.g., if
>> your NOR flash lives above 0xc0000000, you *have* to remap it for the
>> runtime services to be able to use it).
>>
>>
>>
>>> Does such an operating system exist?
>>> Or is this only a hypothetical case?
>>>
>>>>
>>>> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> I am missing a description of the side effects of the change, e.g.
>>>
>>> Our EFI unit tests call the Reset() service.
>>>
>>> So Python tests will fail on systems that do not implement Reset() in a
>>> runtime compatible manner.
>>>
>>
>> All runtime services (except SetVirtualAddresMap() and
>> ConvertPointer(), obviously) may be used at runtime with or without
>> installing a virtual address mapping.
>>
> 
> With this patch in place Travis CI testing fails:
> 
> After 'bootefi selftest' this output is not ejected:
> m = u_boot_console.p.expect(['resetting', 'U-Boot'])
> 
> And after the reset U-Boot crashes (observed for qemu-arm64_defconfig).

I just tried this out on the x86_64 variant. There reset does not occur,
because at the point in time when we trigger the reset,
ExitBootServices() has already been called. I would call that
intentional - we patch the reset function away now because x86 doesn't
provide an explicit efi enabled reset function.

The arm breakage is related to a missing icache flush. I'll fix that one up.


Thanks,

Alex