diff mbox

[edk2,RFC] MdeModulePkg: account for disjoint code and data regions in runtime relocations

Message ID 1435840284-20138-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel July 2, 2015, 12:31 p.m. UTC 
Hello all,

This is a proof of concept patch that fixes the problems that occur when
enabling the EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA
MemoryProtectionAttribute, which may split PE/COFF memory images into disjoint
regions in virtual memory.

It is not a complete solution, but it works both on AARCH64 (ArmVirtQemu) and
X64 (Ovmf). With the 4 KB alignment patch and Laszlo's end-of-DXE series applied,
I can boot into the Linux kernel (Ubuntu 3.13.0-55-generic) without problems,
while the memory map reveals that the splitting has in fact occurred.

Note that this may require 64 KB section alignment for non-ELF based toolchains,
if they may potentially emit EFI_IMAGE_REL_BASED_LOW or EFI_IMAGE_REL_BASED_HIGH
relocations. For toolchains that rely on GenFw for the ELF to PE/COFF
conversion, this is not required.

------------------8<----------------------

When reapplying PE/COFF relocations for the switch to virtual mode, we
should not assume that the virtual memory image is identical to the
PE/COFF file image, since PE/COFF may no longer be adjacent in virtual
memory.

So instead of using a fixed adjustment derived from the virtual address of
ImageBase, apply ConvertPointer () to each relocation target to obtain the
new value.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/RuntimeDxe/Runtime.c | 229 +++++++++++++++++++-
 1 file changed, 227 insertions(+), 2 deletions(-)

Comments

Ard Biesheuvel July 9, 2015, 3:58 p.m. UTC | #1 
On 9 July 2015 at 16:57, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 02 Jul, at 02:31:24PM, Ard Biesheuvel wrote:
>> Hello all,
>>
>> This is a proof of concept patch that fixes the problems that occur when
>> enabling the EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA
>> MemoryProtectionAttribute, which may split PE/COFF memory images into disjoint
>> regions in virtual memory.
>>
>> It is not a complete solution, but it works both on AARCH64 (ArmVirtQemu) and
>> X64 (Ovmf). With the 4 KB alignment patch and Laszlo's end-of-DXE series applied,
>> I can boot into the Linux kernel (Ubuntu 3.13.0-55-generic) without problems,
>> while the memory map reveals that the splitting has in fact occurred.
>>
>> Note that this may require 64 KB section alignment for non-ELF based toolchains,
>> if they may potentially emit EFI_IMAGE_REL_BASED_LOW or EFI_IMAGE_REL_BASED_HIGH
>> relocations. For toolchains that rely on GenFw for the ELF to PE/COFF
>> conversion, this is not required.
>
> Cool! I'd quite like to test this patch out but I'm not sure I've got
> all of the required patch dependencies in my tree.
>
> Could you shove everything into some public git repo so I can run it
> through the LUV autobuilder?
>

Sure. In the meantime, I have cooked up a slightly more elaborate
series that also fixes the
EFI_IMAGE_REL_BASED_LOW/EFI_IMAGE_REL_BASED_HIGH issues (even though
they don't seem to be used widely) and does some cleanups. This is
unlikely to be adopted as-is, since there is still an unresolved issue
with inter-region relative relocations.

Tree is here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/pecoff-runtime-relocation

When running this, you should notice regions in the memory map with
either the RO (0x20000) or XP (0x4000) bit set in the kernel log.
For instance,

"""
[    0.000000] efi: mem38: type=6, attr=0x800000000000400f,
range=[0x0000000007e9a000-0x0000000007e9f000) (0MB)
[    0.000000] efi: mem39: type=5, attr=0x800000000002000f,
range=[0x0000000007e9f000-0x0000000007ea4000) (0MB)
[    0.000000] efi: mem40: type=6, attr=0x800000000000400f,
range=[0x0000000007ea4000-0x0000000007eaa000) (0MB)
[    0.000000] efi: mem41: type=5, attr=0x800000000002000f,
range=[0x0000000007eaa000-0x0000000007eae000) (0MB)
"""

And no crash .... (hopefully)
Ard Biesheuvel July 10, 2015, 1:57 p.m. UTC | #2 
On 10 July 2015 at 15:53, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 09 Jul, at 05:58:36PM, Ard Biesheuvel wrote:
>>
>> Sure. In the meantime, I have cooked up a slightly more elaborate
>> series that also fixes the
>> EFI_IMAGE_REL_BASED_LOW/EFI_IMAGE_REL_BASED_HIGH issues (even though
>> they don't seem to be used widely) and does some cleanups. This is
>> unlikely to be adopted as-is, since there is still an unresolved issue
>> with inter-region relative relocations.
>>
>> Tree is here
>> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/pecoff-runtime-relocation
>
> Thanks Ard! This is really, super helpful.
>

Sure, no problem.

>> When running this, you should notice regions in the memory map with
>> either the RO (0x20000) or XP (0x4000) bit set in the kernel log.
>> For instance,
>>
>> """
>> [    0.000000] efi: mem38: type=6, attr=0x800000000000400f, range=[0x0000000007e9a000-0x0000000007e9f000) (0MB)
>> [    0.000000] efi: mem39: type=5, attr=0x800000000002000f, range=[0x0000000007e9f000-0x0000000007ea4000) (0MB)
>> [    0.000000] efi: mem40: type=6, attr=0x800000000000400f, range=[0x0000000007ea4000-0x0000000007eaa000) (0MB)
>> [    0.000000] efi: mem41: type=5, attr=0x800000000002000f, range=[0x0000000007eaa000-0x0000000007eae000) (0MB)
>> """
>
> Yep, I see this and it looks good.
>
>> And no crash .... (hopefully)
>
> With a standard kernel, I don't see a crash. However, with the minimal
> top 2 patches in,
>
>   https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=memmap
>
> I see a page fault while executing SetVirtualAddressMap(). It appears to
> be caused by the firmware performing relocation fixups on an address
> that's mapped as EfiRuntimeServicesCode, which is in the EFI memory map
> as EFI_MEMORY_RO.
>
> I suspect you don't see this issue on aarch64 because you call
> SetVirtualAddressMap() so early in boot.
>

Indeed.

> I'll try and find some time to dig into this issue this coming week, but
> if anyone wants to beat me to it, feel free ;-)
>
> The first question is: Why is the image's reloc section in a
> EfiRuntimeServiceCode region?
>

Are you sure it is the .reloc section itself? It could well be the
target of a relocation fixup that is inside a code region, which
cannot be applied due to the page permissions.
Ard Biesheuvel July 10, 2015, 2:23 p.m. UTC | #3 
On 10 July 2015 at 16:14, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 10 Jul, at 03:57:52PM, Ard Biesheuvel wrote:
>>
>> Are you sure it is the .reloc section itself? It could well be the
>> target of a relocation fixup that is inside a code region, which
>> cannot be applied due to the page permissions.
>
> Oh yes, that would make more sense. I was under the impression that the
> runtime drivers were built as position independent executables (I don't
> know why I thought that). Whoops!
>
> OK, I've got more kernel work to do then. Looks like we have to first
> call SetVirtualAddressMap() and *then* apply the more restrictive page
> table permissions.
>

Is there a reason to set strict permissions at all on the 1:1 mapping?
Does it stick around after having called SVAM () ?

My (naive) suggestion would be to apply the strict permissions only to
the virtual remapping of the runtime services, but perhaps it doesn't
work like that on x86.
diff mbox

Patch

diff --git a/MdeModulePkg/Core/RuntimeDxe/Runtime.c b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
index c61301cf80d8..c8dbfdd5a3ef 100644
--- a/MdeModulePkg/Core/RuntimeDxe/Runtime.c
+++ b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
@@ -213,6 +213,232 @@  RuntimeDriverConvertInternalPointer (
 }
 
 /**
+  Reapply fixups on a fixed up PE32/PE32+ image to allow virutal calling at EFI
+  runtime.
+
+  This function reapplies relocation fixups to the PE/COFF image specified by ImageBase
+  and ImageSize so the image will execute correctly when the PE/COFF image is mapped
+  to the address specified by VirtualImageBase.  RelocationData must be identical
+  to the FixupData buffer from the PE_COFF_LOADER_IMAGE_CONTEXT structure
+  after this PE/COFF image was relocated with BasePecoffLib::PeCoffLoaderRelocateImage().
+
+  Note that if the platform does not maintain coherency between the instruction cache(s) and the data
+  cache(s) in hardware, then the caller is responsible for performing cache maintenance operations
+  prior to transferring control to a PE/COFF image that is loaded using this funation.
+
+  @param  ImageBase          The base address of a PE/COFF image that has been loaded
+                             and relocated into system memory.
+  @param  ImageSize          The size, in bytes, of the PE/COFF image.
+  @param  RelocationData     A pointer to the relocation data that was collected when the PE/COFF
+                             image was relocated using PeCoffLoaderRelocateImage().
+
+  @return EFI_SUCCESS        The fixups have been reapplied successfully.
+
+  @return EFI_UNSUPPORTED    ImageBase does not point to a valid PE image
+  @return EFI_UNSUPPORTED    No reloc directory found or contents invalid
+  @return EFI_UNSUPPORTED    Image uses high/low relocations and section alignment < 64 KB
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+RuntimeDriverRelocateImageForRuntime (
+  IN  PHYSICAL_ADDRESS        ImageBase,
+  IN  UINTN                   ImageSize,
+  IN  VOID                    *RelocationData
+  )
+{
+  CHAR8                               *OldBase;
+  EFI_IMAGE_DOS_HEADER                *DosHdr;
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+  UINT32                              NumberOfRvaAndSizes;
+  EFI_IMAGE_DATA_DIRECTORY            *DataDirectory;
+  EFI_IMAGE_DATA_DIRECTORY            *RelocDir;
+  EFI_IMAGE_BASE_RELOCATION           *RelocBase;
+  EFI_IMAGE_BASE_RELOCATION           *RelocBaseEnd;
+  UINT16                              *Reloc;
+  UINT16                              *RelocEnd;
+  CHAR8                               *Fixup;
+  CHAR8                               *FixupBase;
+  UINT16                              *Fixup16;
+  UINT32                              *Fixup32;
+  UINT64                              *Fixup64;
+  CHAR8                               *FixupData;
+  UINT16                              Magic;
+  UINTN                               ConvertAddress;
+
+  OldBase = (CHAR8 *)((UINTN)ImageBase);
+
+  //
+  // Find the image's relocate dir info
+  //
+  DosHdr = (EFI_IMAGE_DOS_HEADER *)OldBase;
+  if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
+    //
+    // Valid DOS header so get address of PE header
+    //
+    Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)(((CHAR8 *)DosHdr) + DosHdr->e_lfanew);
+  } else {
+    //
+    // No Dos header so assume image starts with PE header.
+    //
+    Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)OldBase;
+  }
+
+  if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
+    //
+    // Not a valid PE image so Exit
+    //
+    return EFI_UNSUPPORTED;
+  }
+
+  Magic = Hdr.Pe32->OptionalHeader.Magic;
+
+  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    //
+    // Use PE32 offset
+    //
+    NumberOfRvaAndSizes = Hdr.Pe32->OptionalHeader.NumberOfRvaAndSizes;
+    DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32->OptionalHeader.DataDirectory[0]);
+  } else {
+    //
+    // Use PE32+ offset
+    //
+    NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
+    DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
+  }
+
+  //
+  // Find the relocation block
+  //
+  // Per the PE/COFF spec, you can't assume that a given data directory
+  // is present in the image. You have to check the NumberOfRvaAndSizes in
+  // the optional header to verify a desired directory entry is there.
+  //
+  if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
+    RelocDir      = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC;
+    RelocBase     = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + RelocDir->VirtualAddress);
+    RelocBaseEnd  = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + RelocDir->VirtualAddress + RelocDir->Size);
+  } else {
+    //
+    // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
+    //
+    ASSERT (FALSE);
+    return EFI_UNSUPPORTED;
+  }
+
+  //
+  // ASSERT for the invalid image when RelocBase and RelocBaseEnd are both NULL.
+  //
+  ASSERT (RelocBase != NULL && RelocBaseEnd != NULL);
+
+  //
+  // Run the whole relocation block. And re-fixup data that has not been
+  // modified. The FixupData is used to see if the image has been modified
+  // since it was relocated. This is so data sections that have been updated
+  // by code will not be fixed up, since that would set them back to
+  // defaults.
+  //
+  FixupData = RelocationData;
+  while (RelocBase < RelocBaseEnd) {
+    //
+    // Add check for RelocBase->SizeOfBlock field.
+    //
+    if ((RelocBase->SizeOfBlock == 0) || (RelocBase->SizeOfBlock > RelocDir->Size)) {
+      //
+      // Data invalid, cannot continue to relocate the image, just return.
+      //
+      return EFI_UNSUPPORTED;
+    }
+
+    Reloc     = (UINT16 *) ((UINT8 *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
+    RelocEnd  = (UINT16 *) ((UINT8 *) RelocBase + RelocBase->SizeOfBlock);
+    FixupBase = (CHAR8 *) ((UINTN)ImageBase) + RelocBase->VirtualAddress;
+
+    //
+    // Run this relocation record
+    //
+    while (Reloc < RelocEnd) {
+
+      Fixup = FixupBase + (*Reloc & 0xFFF);
+      switch ((*Reloc) >> 12) {
+
+      case EFI_IMAGE_REL_BASED_ABSOLUTE:
+        break;
+
+      case EFI_IMAGE_REL_BASED_HIGH:
+        //
+        // In order to be able to apply relocations to code and data regions
+        // that may be disjoint in the virtual mapping, we disallow relocations
+        // of type EFI_IMAGE_REL_BASED_HIGH or EFI_IMAGE_REL_BASED_LOW, unless
+        // the section alignment is at least 64 KB.
+        //
+        if (Hdr.Pe32->OptionalHeader.SectionAlignment < SIZE_64KB) {
+          return EFI_UNSUPPORTED;
+        }
+        Fixup16 = (UINT16 *) Fixup;
+        if (*(UINT16 *) FixupData == *Fixup16) {
+          ConvertAddress = (UINTN) *Fixup16 << 16;
+          RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+          *Fixup16 = (UINT16) (ConvertAddress >> 16);
+        }
+
+        FixupData = FixupData + sizeof (UINT16);
+        break;
+
+      case EFI_IMAGE_REL_BASED_LOW:
+        if (Hdr.Pe32->OptionalHeader.SectionAlignment < SIZE_64KB) {
+          return EFI_UNSUPPORTED;
+        }
+        //
+        // Since the adjustment is guaranteed to be a multiple of 64 KB,
+        // there is nothing to do here.
+        //
+        break;
+
+      case EFI_IMAGE_REL_BASED_HIGHLOW:
+        Fixup32       = (UINT32 *) Fixup;
+        FixupData = ALIGN_POINTER (FixupData, sizeof (UINT32));
+        if (*(UINT32 *) FixupData == *Fixup32) {
+          ConvertAddress = (UINTN) *Fixup32;
+          RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+          *Fixup32 = (UINT32) ConvertAddress;
+        }
+
+        FixupData = FixupData + sizeof (UINT32);
+        break;
+
+      case EFI_IMAGE_REL_BASED_DIR64:
+        Fixup64       = (UINT64 *)Fixup;
+        FixupData = ALIGN_POINTER (FixupData, sizeof (UINT64));
+        if (*(UINT64 *) FixupData == *Fixup64) {
+          ConvertAddress = (UINTN) *Fixup64;
+          RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+          *Fixup64 = (UINT64) ConvertAddress;
+        }
+
+        FixupData = FixupData + sizeof (UINT64);
+        break;
+
+      default:
+        ASSERT (FALSE);
+      }
+      //
+      // Next relocation record
+      //
+      Reloc += 1;
+    }
+    //
+    // next reloc block
+    //
+    RelocBase = (EFI_IMAGE_BASE_RELOCATION *) RelocEnd;
+  }
+
+  return EFI_SUCCESS;
+}
+
+
+/**
 
   Changes the runtime addressing mode of EFI firmware from physical to virtual.
 
@@ -312,9 +538,8 @@  RuntimeDriverSetVirtualAddressMap (
       Status  = RuntimeDriverConvertPointer (0, (VOID **) &VirtImageBase);
       ASSERT_EFI_ERROR (Status);
 
-      PeCoffLoaderRelocateImageForRuntime (
+      RuntimeDriverRelocateImageForRuntime (
         (EFI_PHYSICAL_ADDRESS) (UINTN) RuntimeImage->ImageBase,
-        VirtImageBase,
         (UINTN) RuntimeImage->ImageSize,
         RuntimeImage->RelocationData
         );