[U-Boot] [RFC 1/6] efi_loader: runtime: make SetVirtualAddressMap configurable

[Mark Kettenis]

> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Date: Sun, 16 Jun 2019 23:52:49 +0200
> 
> On 6/15/19 11:14 PM, Mark Kettenis wrote:
> >> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> Date: Sat, 15 Jun 2019 21:46:02 +0200
> >>
> >> On 6/5/19 6:21 AM, AKASHI Takahiro wrote:
> >>> OS does not always need to call SetVirtualAddressMap.
> >>> (Ard confirmed this on arm64 linux.)
> >>> So let this API configurable. If disabled, it will return EFI_UNSUPPORTED
> >>> as UEFI specification requires.
> >>
> >> Currently we do not support this scenario. Alex's patch should go in first.
> >
> > OpenBSD/arm64 will always call this function.  It does this in order
> 
> OpenBSD/arm32 does not call it. I have no clue why the 32bit and 64bit
> code have diverged.

Beyond the device drivers, there isn't much shared code between
OpenBSD/armv7 and OpenBSD/arm64.  And the code that handles EFI
runtime services is tied quite closely to the memory management code
which is quite different between AArch32 and AArch64.  The primary
reason for adding support for EFI runtime services to OpenBSD/amd64
was to support getting and setting the time on machines with
"fullblown" UEFI support.  Machines that have armv7 CPUs *and*
fullblown UEFI support are pretty much non-existent so I didn't
implement support for armv7 yet.

> > to randomize the virtual addresses used by runtime services to make it
> > harder for an attacker to call into UEFI runtime services.  Note that
> > the UEFI 2.7 standard provides no indication that this interface might
> > be optional, so I don't think OpenBSD is doing anything wrong here.
> >
> > I think it is unwise to make this API configurable.  But disabling it
> > by default like this diff does would be a seriously bad idea.  It
> > means U-Boot would no longer be backwards compatible with UEFI 2.7.
> 
> The current UEFI specification is 2.8. It foresees that
> SetVirtualAddressMap() can be marked as unsupported but that does not
> imply that we have to make it customizable. And disabling it by default
> does not make much sense while most operating systems use it.

Right!  Thanks,

Mark

> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> >>> ---
> >>>    lib/efi_loader/Kconfig       | 7 +++++++
> >>>    lib/efi_loader/efi_runtime.c | 8 ++++++++
> >>>    2 files changed, 15 insertions(+)
> >>>
> >>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >>> index 8bf4b1754d06..bb9c7582b14d 100644
> >>> --- a/lib/efi_loader/Kconfig
> >>> +++ b/lib/efi_loader/Kconfig
> >>> @@ -44,6 +44,13 @@ config EFI_SET_TIME
> >>>    	  Provide the SetTime() runtime service at boottime. This service
> >>>    	  can be used by an EFI application to adjust the real time clock.
> >>>
> >>> +config EFI_RUNTIME_SET_VIRTUAL_ADDRESS_MAP
> >>> +	bool "runtime service: SetVirtualAddressMap"
> >>> +	default n
> >>> +	help
> >>> +	  Enable SetVirtualAddressMap runtime service. This API will be
> >>> +	  called by OS just before it enters into virtual address mode.
> >>> +
> >>>    config EFI_DEVICE_PATH_TO_TEXT
> >>>    	bool "Device path to text protocol"
> >>>    	default y
> >>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >>> index 9c50955c9bd0..60442cb21d37 100644
> >>> --- a/lib/efi_loader/efi_runtime.c
> >>> +++ b/lib/efi_loader/efi_runtime.c
> >>> @@ -374,10 +374,12 @@ static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
> >>>    		/* do_reset is gone */
> >>>    		.ptr = &efi_runtime_services.reset_system,
> >>>    		.patchto = efi_reset_system,
> >>> +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP
> >>>    	}, {
> >>>    		/* invalidate_*cache_all are gone */
> >>>    		.ptr = &efi_runtime_services.set_virtual_address_map,
> >>>    		.patchto = &efi_unimplemented,
> >>> +#endif
> >>>    	}, {
> >>>    		/* RTC accessors are gone */
> >>>    		.ptr = &efi_runtime_services.get_time,
> >>> @@ -512,6 +514,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map)
> >>>            invalidate_icache_all();
> >>>    }
> >>>
> >>> +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP
> >>>    /**
> >>>     * efi_set_virtual_address_map() - change from physical to virtual mapping
> >>>     *
> >>> @@ -619,6 +622,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
> >>>
> >>>    	return EFI_EXIT(EFI_INVALID_PARAMETER);
> >>>    }
> >>> +#endif /* CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP */
> >>>
> >>>    /**
> >>>     * efi_add_runtime_mmio() - add memory-mapped IO region
> >>> @@ -796,7 +800,11 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = {
> >>>    	.set_time = &efi_set_time_boottime,
> >>>    	.get_wakeup_time = (void *)&efi_unimplemented,
> >>>    	.set_wakeup_time = (void *)&efi_unimplemented,
> >>> +#ifdef CONFIG_RUNTIME_SET_VIRTUAL_ADDRESS_MAP
> >>>    	.set_virtual_address_map = &efi_set_virtual_address_map,
> >>> +#else
> >>> +	.set_virtual_address_map = (void *)&efi_unimplemented,
> >>
> >> Depending on the ABI it is not save to use a function with another set
> >> of parameters.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +#endif
> >>>    	.convert_pointer = (void *)&efi_invalid_parameter,
> >>>    	.get_variable = efi_get_variable,
> >>>    	.get_next_variable_name = efi_get_next_variable_name,
> >>>
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> https://lists.denx.de/listinfo/u-boot
> >
> 
>