Driver model at UEFI runtime

Thu Sep 30 13:49:40 CEST 2021

On 9/30/21 13:13, Mark Kettenis wrote:
>> From: Simon Glass <sjg at chromium.org>
>> Date: Wed, 29 Sep 2021 22:08:49 -0600
>
> Hi Simon,
>
>> Hi Heinrich,
>>
>> On Fri, 10 Sept 2021 at 08:19, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>
>>>
>>>
>>> On 9/9/21 10:00 PM, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Thu, 9 Sept 2021 at 05:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>>>
>>>>> Hello Simon,
>>>>>
>>>>> The EBBR specification requires that the UEFI SystemReset() runtime
>>>>> service is available to the operating system.
>>>>>
>>>>> Up to now this has been implemented by overriding function
>>>>> efi_reset_system() which is marked as __efi_runtime.
>>>>>
>>>>> Both ARM and RISC-V support generic interfaces for reset. PSCI for ARM
>>>>> and the System Reset Extension for SBI on RISC-V. This has kept the
>>>>> number of implementations low. The only exceptions are:
>>>>>
>>>>> * arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>>>>> * arch/arm/mach-bcm283x/reset.c for the Raspberry PIs
>>>>> * arch/sandbox/cpu/start.c
>>>>>
>>>>> Bin has suggested in
>>>>> https://lists.denx.de/pipermail/u-boot/2021-September/459865.html to use
>>>>> reset drivers based on the driver model.
>>>>>
>>>>> Currently after ExitBootServices() the driver model does not exist anymore.
>>>>>
>>>>> When evaluating Bin's suggestion one has to keep in mind that after
>>>>> invoking ExitBootServices() most operating systems call
>>>>> SetVirtualAddressMap(). Due to the change of the address map all
>>>>> pointers used by U-Boot afterwards must be updated to match the new
>>>>> memory map.
>>>>>
>>>>> The impression that Ilias and I have is that keeping the driver model
>>>>> alive after SetVirtualAddressMap() would incur:
>>>>>
>>>>> * a high development effort
>>>>> * a significant code size increase
>>>>> * an enlarged attack surface
>>>>>
>>>>> For RISC-V it has been clarified in the platform specification that the
>>>>> SBI must implement the system reset extension. For ARMv8 using TF-A and
>>>>> PSCI is what ARM suggests.
>>>>>
>>>>> So for these architectures we do not expect a growth in the number of
>>>>> drivers needed.
>>>>>
>>>>> Ilias and my favorite would be keeping the design as is.
>>>>>
>>>>> What is your view on this?
>>>>
>>>> Not to dump on the original author but here again we are paying the
>>>> price for the shortcuts taken at the time and not since revisited.
>>>>
>>>> My original request then was to create a new build of U-Boot, since we
>>>> need to build (and load) the runtime stuff separately. Then we can
>>>
>>> Do you mean by new build something like TPL, SPL?
>>
>> I suppose, but we need to move it to PHASE instead, I think. BTW I
>> sent a series that shows how we can drop TPL_SPL_ once we complete the
>> CONFIG migration.
>>
>>>
>>> Tom is right in complaining that the UEFI implementation is getting too
>>> big for some boards. Duplicating a lot of binary code, e.g. the complete
>>> libfdt or everything needed for UEFI variables, does not look a viable
>>> option. The good thing about tagging functions as __efi_runtime is
>>> minimizing binary code duplication.
>>
>> That's true, but it is going to become impossible to maintain this
>> mess at some point. For example there is a duplicated reset driver and
>> the UEFI runtime specifically avoiding using driver model. Where does
>> it end?!
>
> It ends with whatever functionality we decide that the EFI runtime has
> to support after ExitBootServices() has been called.  In EBBR all
> runtime services are optional at this stage, with the exception of
> SetVirtualAddressMap() and ConvertPointer(). So the only thing that
> you really need is a dummy implementation that returns EFI_UNSUPPORTED
> for everything else and a way to relocate that implementation to a
> virtual address chosen by the OS.  So not even a reset implementation
> is needed.  And then there is no reason to get DM involved.
>
> Of course the OS will need some way to reset/shutdown the machine.  In
> principle this is supposed to be implemented in other firmware
> components (PSCI on ARM, SBI on RISC-V) and the UEFI implementation is
> supposed to just make the appropriate firmware call.  In that case I'd
> say implementing ResetSystem() would be so trivial that involving DM
> makes very little sense.  Unfortunately not all PSCI/SBI
> implementations actually implement this.  But even then, as long as
> the hardware needed to reset the system is exposed in the DT passed to
> the OS, the OS can just take care of this itself.
>
> It's the capsule update stuff and EFI variables stuff that creates
> complications if you want to support those while the OS is running.
> But the former doesn't really work without the latter and I fear that
> implementing EFI variables is just not possible on the majority of the
> boards we support in U-Boot.  Especially if you care about verified
> boot.  You pretty much have to have something like OP-TEE running in a
> secure enclave to do this properly.  There is a good reason why this
> was made optional in EBBR.

UEFI variables are already implemented. After ExitBootServices() we
support GetVariable() and GetNextVeriableName(). SetVariable() is not
yet implemented.

>
>> IMO EFI runtime is its own binary and we're going to have to accept
>> that at some point.
>
> Yes.  I think so.  Just like we do for the PSCI implementation that is
> provided for some of the boards that don't come with a TF-A
> implementation.  The minimal implementation I suggest above would be
> tiny and would fit very well in this model.  My suggestion would be
> that before ExitBootServices() gets called, runtime services would
> just call normal U-Boot code using DM and all.  And when
> ExitBootServices() gets called we just switch in the standalone dummy
> implementation.

This switch is currently done in efi_runtime_detach() and in event
handlers for the ExitBootService event.

>
> Personally, I would really prefer this approach where the runtime code
> is as minimal as possible.  It would be way easier to audit and
> convince myself as an OS developer that yes, if I make an EFI runtime
> call it isn't going to do nasty stuff behind my back.

These are the *.c files containing __efi_runtime code.

arch/arm/cpu/armv8/fsl-layerscape/cpu.c
arch/arm/lib/sections.c
arch/arm/mach-bcm283x/reset.c
arch/riscv/lib/sbi.c (if we support runtime reset)
arch/sandbox/cpu/start.c
arch/sandbox/lib/sections.c
arch/x86/lib/sections.c
drivers/firmware/psci.c
drivers/sysreset/sysreset_sbi.c (if we support runtime reset)
drivers/sysreset/sysreset_x86.c
lib/charset.c
lib/crc32.c
lib/efi_loader/efi_boottime.c
lib/efi_loader/efi_memory.c
lib/efi_loader/efi_runtime.c
lib/efi_loader/efi_var_common.c
lib/efi_loader/efi_var_mem.c
lib/efi_loader/efi_variable.c
lib/efi_loader/efi_variable_tee.c

>
>>> It would be possible to leave the whole U-Boot binary in memory when
>>> launching the operating system at the cost of loosing < 1MiB of RAM.
>>> This could eliminate the __efi_runtime tagging.
>>
>> Yes, but will people complain about the size?
>
> Yes.  This would leave a ton of executable code in memory that could
> be exploited using a kernel ROP attack.

So let's exclude this option.

Best regards

Heinrich

>
>>> The problematic stuff are the memory structures that we need to convey
>>> between the boottime and the runtime. It is here where pointers need to
>>> be updated. You cannot resolve this data side problem by duplicating code.
>>>
>>> The first thing we should work on is an easily parsable structure
>>> without pointers for conveying runtime device information.
>>>
>>> Something like a concatenation of structures with
>>>
>>> * length
>>> * driver id
>>> * private data
>>>
>>> might be sufficient.
>>
>> Well yes that's a whole other problem. I suppose we ultimately end up
>> running dm_init() again when we start up the runtime? :-(
>>
>>>
>>>> avoid all this mess and just use the normal U-Boot code (and driver
>>>> model). It also scales up to whatever else we want to do to the runtime
>>>> stuff in the future.
>>>>
>>>> This will be somewhat easier with the VPL series applied, and even
>>>
>>> VPL? Please, provide a link.
>>
>> http://patchwork.ozlabs.org/project/uboot/list/?series=263034
>>
>>>
>>> When thinking of which drivers are needed at runtime it is restricted to
>>> the following:
>>>
>>> * reset driver. Some like SBI can be blindly called at runtime because
>>>     configuration tells us that there is an SBI. For others, e.g GPIO,
>>>     we need information from the runtime devicetree. For others we may
>>>     want the result of probing at boottime to avoid code duplication.
>>>
>>> * tee driver: for managing variables at runtime it would be
>>>     good to have access to non-volatile memory managed by the TEE.
>>>     This has not been realized yet.
>>>
>>> All devices that are managed by the operating system must not be touched
>>> by the runtime firmware.
>>
>> OK.
>>
>> My eyes are glazing over at this point. As you say, EFI runtime as a
>> separate binary will further increase the size of EFI, but we will end
>> up there in the end as people need to call more U-Boot code. No one
>> ever claimed that EFI was svelte.
>>
>> Regards,
>> Simon
>>