Re: [PATCH edk2-platforms 1/1] Platform/ARM/ArmShellCmdRunAxf: switch to by-VA cache maintenance

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 4 Mar 2020 at 12:47, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Mar 04, 2020 at 12:42:50 +0100, Ard Biesheuvel wrote:
> > Currently, the 'runaxf' shell command that exists only on ARM's own
> > development platforms deals with the caches in an unsafe manner, as
> > it relies on set/way maintenance to ensure that the ELF image it has
> > put into memory, as well as the running image itself is visible in
> > memory after the MMU and caches are disabled.
> >
> > So let's switch to by-VA maintenance for the currently running image,
> > as well as the ELF image, and use a helper in assembly to ensure that
> > we are not relying on the stack between the disabling of the MMU and
> > the invocation of the ELF image.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Thanks

One thing occurred to me though, please see below

> > ---
> >  Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S       | 40 +++++++++
> >  Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S           | 40 +++++++++
> >  Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf |  8 ++
> >  Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c              | 91 +++++++++-----------
> >  4 files changed, 131 insertions(+), 48 deletions(-)
> >
> > diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S b/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S
> > new file mode 100644
> > index 000000000000..dd7aa48f7a02
> > --- /dev/null
> > +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S
> > @@ -0,0 +1,40 @@
> > +//
> > +//  Copyright (c) 2020, ARM Limited. All rights reserved.
> > +//
> > +//  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +
> > +#include <AsmMacroIoLibV8.h>
> > +
> > +// VOID
> > +// RunAxfPivot (
> > +//   IN  ELF_ENTRYPOINT  ElfEntry
> > +//   IN  UINTN           Arg0,
> > +//   IN  UINTN           Arg1,
> > +//   IN  UINTN           Arg2,
> > +//   IN  UINTN           Arg3
> > +//   );
> > +ASM_FUNC(RunAxfPivot)
> > +  // Preserve ElfEntry() and its arguments
> > +  // Since we will not be returning from this function, we can clobber
> > +  // callee preserved register instead.
> > +  mov   x19, x0
> > +  mov   x20, x1
> > +  mov   x21, x2
> > +  mov   x22, x3
> > +  mov   x23, x4
> > +
> > +  bl    ArmDisableMmu
> > +
> > +  // Load ElfEntry()'s arguments into x0...x3
> > +  mov   x0, x20
> > +  mov   x1, x21
> > +  mov   x2, x22
> > +  mov   x3, x23
> > +
> > +  // Call ElfEntry()
> > +  blr   x19
> > +
> > +0:wfi
> > +  wfe
> > +  b     0b
> > diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S b/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S
> > new file mode 100644
> > index 000000000000..edb3505e5cb9
> > --- /dev/null
> > +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S
> > @@ -0,0 +1,40 @@
> > +//
> > +//  Copyright (c) 2020, ARM Limited. All rights reserved.
> > +//
> > +//  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +
> > +#include <AsmMacroIoLibV8.h>
> > +
> > +// VOID
> > +// RunAxfPivot (
> > +//   IN  ELF_ENTRYPOINT  ElfEntry
> > +//   IN  UINTN           Arg0,
> > +//   IN  UINTN           Arg1,
> > +//   IN  UINTN           Arg2,
> > +//   IN  UINTN           Arg3
> > +//   );
> > +ASM_FUNC(RunAxfPivot)
> > +  // Preserve ElfEntry() and its arguments without using the stack.
> > +  // Since we will not be returning from this function, we can clobber
> > +  // callee preserved register instead.
> > +  mov   r4, r0
> > +  mov   r5, r1
> > +  mov   r6, r2
> > +  mov   r7, r3
> > +  pop   {r8}
> > +
> > +  bl    ArmDisableMmu
> > +
> > +  // Load ElfEntry()'s arguments into x0...x3
> > +  mov   r0, r5
> > +  mov   r1, r6
> > +  mov   r2, r7
> > +  mov   r3, r8
> > +
> > +  // Call ElfEntry()
> > +  blx   r4
> > +
> > +0:wfi
> > +  wfe
> > +  b     0b
> > diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf b/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> > index 74c6a0e84fdf..7c27a765bd5c 100644
> > --- a/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> > +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> > @@ -27,6 +27,12 @@ [Sources.common]
> >    elf64.h
> >    elf_common.h
> >
> > +[Sources.AARCH64]
> > +  AArch64/Pivot.S
> > +
> > +[Sources.ARM]
> > +  Arm/Pivot.S
> > +
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    MdeModulePkg/MdeModulePkg.dec
> > @@ -37,11 +43,13 @@ [Packages]
> >  [LibraryClasses]
> >    ArmLib
> >    BaseLib
> > +  CacheMaintenanceLib
> >    DebugLib
> >    HiiLib
> >    ShellLib
> >
> >  [Protocols]
> > +  gEfiLoadedImageProtocolGuid
> >    gEfiShellDynamicCommandProtocolGuid
> >
> >  [Guids]
> > diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c b/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> > index 153aed2ab6bd..efad5c4525ac 100644
> > --- a/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> > +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> > @@ -10,6 +10,7 @@
> >
> >  #include <Guid/GlobalVariable.h>
> >
> > +#include <Library/CacheMaintenanceLib.h>
> >  #include <Library/PrintLib.h>
> >  #include <Library/HandleParsingLib.h>
> >  #include <Library/DevicePathLib.h>
> > @@ -20,6 +21,8 @@
> >
> >  #include <Library/ArmLib.h>
> >
> > +#include <Protocol/LoadedImage.h>
> > +
> >  #include "ArmShellCmdRunAxf.h"
> >  #include "ElfLoader.h"
> >  #include "BootMonFsLoader.h"
> > @@ -85,37 +88,21 @@ ShutdownUefiBootServices (
> >    return Status;
> >  }
> >
> > -
> > -STATIC
> > -EFI_STATUS
> > -PreparePlatformHardware (
> > -  VOID
> > -  )
> > -{
> > -  //Note: Interrupts will be disabled by the GIC driver when ExitBootServices() will be called.
> > -
> > -  // Clean before Disable else the Stack gets corrupted with old data.
> > -  ArmCleanDataCache ();
> > -  ArmDisableDataCache ();
> > -  // Invalidate all the entries that might have snuck in.
> > -  ArmInvalidateDataCache ();
> > -
> > -  // Disable and invalidate the instruction cache
> > -  ArmDisableInstructionCache ();
> > -  ArmInvalidateInstructionCache ();
> > -
> > -  // Turn off MMU
> > -  ArmDisableMmu();
> > -
> > -  return EFI_SUCCESS;
> > -}
> > -
> >  // Process arguments to pass to AXF?
> >  STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> >    {NULL, TypeMax}
> >  };
> >
> >
> > +VOID
> > +RunAxfPivot (
> > +  IN  ELF_ENTRYPOINT  ElfEntry,
> > +  IN  UINTN           Arg0,
> > +  IN  UINTN           Arg1,
> > +  IN  UINTN           Arg2,
> > +  IN  UINTN           Arg3
> > +  );
> > +
> >  /**
> >    This is the shell command handler function pointer callback type. This
> >    function handles the command when it is invoked in the shell.
> > @@ -139,23 +126,23 @@ ShellDynCmdRunAxfHandler (
> >    IN EFI_SHELL_PROTOCOL                    *Shell
> >    )
> >  {
> > -  LIST_ENTRY        *ParamPackage;
> > -  EFI_STATUS         Status;
> > -  SHELL_STATUS       ShellStatus;
> > -  SHELL_FILE_HANDLE  FileHandle;
> > -  ELF_ENTRYPOINT     StartElf;
> > -  CONST CHAR16      *FileName;
> > -  EFI_FILE_INFO     *Info;
> > -  UINTN              FileSize;
> > -  VOID              *FileData;
> > -  VOID              *Entrypoint;
> > -  LIST_ENTRY         LoadList;
> > -  LIST_ENTRY        *Node;
> > -  LIST_ENTRY        *NextNode;
> > -  RUNAXF_LOAD_LIST  *LoadNode;
> > -  CHAR16            *TmpFileName;
> > -  CHAR16            *TmpChar16;
> > -
> > +  LIST_ENTRY                  *ParamPackage;
> > +  EFI_STATUS                  Status;
> > +  SHELL_STATUS                ShellStatus;
> > +  SHELL_FILE_HANDLE           FileHandle;
> > +  ELF_ENTRYPOINT              StartElf;
> > +  CONST CHAR16                *FileName;
> > +  EFI_FILE_INFO               *Info;
> > +  UINTN                       FileSize;
> > +  VOID                        *FileData;
> > +  VOID                        *Entrypoint;
> > +  LIST_ENTRY                  LoadList;
> > +  LIST_ENTRY                  *Node;
> > +  LIST_ENTRY                  *NextNode;
> > +  RUNAXF_LOAD_LIST            *LoadNode;
> > +  CHAR16                      *TmpFileName;
> > +  CHAR16                      *TmpChar16;
> > +  EFI_LOADED_IMAGE_PROTOCOL   *LoadedImage;
> >
> >    ShellStatus = SHELL_SUCCESS;
> >    FileHandle = NULL;
> > @@ -291,6 +278,11 @@ ShellDynCmdRunAxfHandler (
> >      }
> >    }
> >
> > +  //
> > +  // Clean the loaded file to the PoC
> > +  //
> > +  WriteBackDataCacheRange (FileData, FileSize);
> > +

Since we are no longer touching the I-cache now, I should do a

InvalidateInstructionCacheRange (FileData, FileSize);

here as well. Mind if I fold that in?


> >    // Program load list created.
> >    // Shutdown UEFI, copy and jump to code.
> >    if (!IsListEmpty (&LoadList) && !EFI_ERROR (Status)) {
> > @@ -315,14 +307,17 @@ ShellDynCmdRunAxfHandler (
> >          Node = GetNextNode (&LoadList, Node);
> >        }
> >
> > -      //
> > -      // Switch off interrupts, caches, mmu, etc
> > -      //
> > -      Status = PreparePlatformHardware ();
> > +      Status = gBS->HandleProtocol (gImageHandle, &gEfiLoadedImageProtocolGuid,
> > +                      (VOID **)&LoadedImage);
> >        ASSERT_EFI_ERROR (Status);
> >
> > -      StartElf = (ELF_ENTRYPOINT)Entrypoint;
> > -      StartElf (0,0,0,0);
> > +      //
> > +      // Ensure that the currently running image is clean to the PoC so we can
> > +      // safely keep executing it with the MMU and caches off
> > +      //
> > +      WriteBackDataCacheRange (LoadedImage->ImageBase, LoadedImage->ImageSize);
> > +
> > +      RunAxfPivot (StartElf, 0, 0, 0, 0);
> >
> >        // We should never get here.. But if we do, spin..
> >        ASSERT (FALSE);
> > --
> > 2.17.1
> >